pr-review

name	pr-review
description	Review Velox pull requests for code quality, memory safety, performance, and correctness. Use when reviewing PRs, when asked to review code changes, or when the user mentions "/pr-review".

Velox PR Review Skill

Review Velox pull requests focusing on what CI cannot check: code quality, memory safety, concurrency, performance, and correctness. This is performance-critical C++ code for a database execution engine where bugs can cause data corruption, crashes, or security vulnerabilities.

Usage Modes

GitHub Actions Mode

When invoked via /pr-review [additional context] on a GitHub PR, the action pre-fetches PR metadata and injects it into the prompt. Detect this mode by the presence of <formatted_context>, <pr_or_issue_body>, and <comments> tags in the prompt.

The prompt already contains:

• PR metadata (title, author, branch names, additions/deletions, file count)
• PR body/description
• All comments and review comments (with file/line references)
• List of changed files with paths and change types

Use git commands to get the diff and commit history. The base branch name is in the prompt context (look for PR Branch: -> or the baseBranch field).

# Get the full diff against the base branch
git diff origin/<baseBranch>...HEAD

# Get diff stats
git diff --stat origin/<baseBranch>...HEAD

# Get commit history for this PR
git log origin/<baseBranch>..HEAD --oneline

# If the base branch ref is not available, fetch it first
git fetch origin <baseBranch> --depth=1

Do NOT use gh CLI commands in this mode -- only git commands are available. All PR metadata, comments, and reviews are already in the prompt context; only the diff and commit log need to be fetched via git.

If the reviewer provided additional context or instructions after the /pr-review command, incorporate those into your review focus.

Local CLI Mode

The user provides a PR number or URL:

/pr-review 12345
/pr-review https://github.com/facebookincubator/velox/pull/12345

Use gh CLI to fetch PR data:

gh pr view <PR_NUMBER> --json title,body,author,baseRefName,headRefName,files,additions,deletions,commits
gh pr diff <PR_NUMBER>
gh pr view <PR_NUMBER> --json comments,reviews

Review Workflow

Step 1: Read Project Guidelines

Before reviewing, you MUST Read CODING_STYLE.md at the repo root in full. Do not skim, do not skip — every modified line in the diff must be checked against it.

The "Common Mistakes" section is the authoritative checklist for the highest-volume real review hits (/// vs //, abbreviations, *Utils, undocumented headers, header-body weight, goto, test-first for bug fixes, naming conventions, assert forms, etc.).

This skill does not maintain a duplicate checklist — CODING_STYLE.md is the single source of truth. If anything in this skill ever appears to contradict CODING_STYLE.md, prefer CODING_STYLE.md.

Step 2: Analyze Changes and Prior Review

Read through the diff systematically:

1. Identify the purpose of the change from title/description
2. Group changes by type (new code, tests, config, docs)
3. Note the scope of changes (files affected, lines changed)

The <comments> block in the prompt context contains all prior review comments — including any from earlier /pr-review invocations on this PR. Read them before reviewing:

• Do not re-flag issues already raised by a prior reviewer (human or Claude). Re-flagging trains authors to ignore Claude reviews.
• If a prior comment was addressed by a follow-up commit, verify the fix in the diff rather than restating the original concern.
• If /pr-review was invoked in reply to a specific comment thread, focus the review on that thread's concerns instead of re-reviewing the whole PR.

Step 3: Check PR Title and Description Quality

Read the PR title and body, and walk the self-check in .claude/skills/write-commit-message/SKILL.md. If 2 or more items fail (e.g., long lists embedded in sentences, function-by-function walkthroughs, restating the diff, jargon nouns, long inline code/error strings), include a single short note in the summary comment asking the author to tighten the description. Template:

The PR description would read more clearly with a rewrite. Specifically:
- <issue 1, one short sentence>
- <issue 2, one short sentence>

The `write-commit-message` skill at `.claude/skills/write-commit-message/`
can help (it has a self-check + drafting workflow), but any path is fine
as long as the result is clear.

One short paragraph in the summary comment — do not file an inline comment per issue, and do not nag on 0–1 minor issues.

Step 4: Deep Review

Trace the logic step by step. For each change, consider boundary conditions (empty, null, max size, first/last iteration), failure modes (allocation failures, exceptions, partial state), concurrency (race conditions, lock ordering), and memory safety (ownership, lifetimes, dangling references). Be strict — better to flag a potential issue than miss a real bug. The Review Areas table below enumerates what to check; do not duplicate it as narrative.

Review Areas

Analyze each of these areas thoroughly:

Area	Focus
Correctness & Edge Cases	Logic errors, off-by-one, null/empty handling, boundary conditions, integer overflow, floating point edge cases (NaN, Inf, negative zero)
Memory Safety	Use-after-free, double-free, leaks, dangling pointers/references, buffer overflows, ownership/lifetime issues, exception safety
Concurrency	Race conditions, data races, deadlocks, lock ordering, thread-safety of shared state
Performance	Unnecessary copies (move semantics?), inefficient algorithms, cache-unfriendly access, excessive allocations in hot paths
Error Handling	All error paths handled? Exceptions caught appropriately? Informative error messages? Correct use of VELOX_CHECK_* vs VELOX_USER_CHECK_*?
Code Quality	RAII, const-correctness, smart pointers, naming conventions, clear structure
Testing	Sufficient tests? Edge cases covered? Error paths tested? Using gtest matchers? Bug-fix PRs: does the diff add a test that would fail without the fix? Flag bug fixes that ship code-only.

Output Format

The output should be a markdown-formatted summary and should follow the following markdown format exactly:

### Summary
Brief overall assessment (1-2 sentences)

### Issues Found
List any issue, categorized by severity:
 - 🔴 **Critical**: Must fix before merge
 - 🟡 **Suggestion**: Design, structure, and maintainability concerns —
   abstraction boundaries, duplicated ownership, over-general APIs, coupling
   that undercuts the PR's goal. "It works" does not downgrade these.
 - 🟢 **Nitpick**: Cosmetic only — typos, formatting, a single local name.
   Structural concerns are never 🟢.

Each issue should also include:
- File and line reference
- Description of the issue
- Suggested fix if applicable

### Positive Observations
Note any particularly good patterns or improvements.

Inline Comments

Use the mcp__github_inline_comment__create_inline_comment tool to post comments directly on specific lines in the PR diff. Inline comments should be used whenever pointing at the exact line adds clarity beyond the summary comment.

Use inline comments for:

• Concrete bugs or incorrect logic
• Memory safety issues (use-after-free, dangling references, leaks)
• Off-by-one errors or boundary condition mistakes
• Incorrect use of VELOX_CHECK_* vs VELOX_USER_CHECK_*

Do NOT use inline comments for:

• Style nitpicks or naming suggestions
• General architectural feedback
• Positive observations
• Anything that applies broadly rather than to a specific line

Always post a summary comment with the overall review. Inline comments supplement the summary — they do not replace it.

Key Principles

1. No repetition - Each observation appears in exactly one place
2. Focus on what CI cannot check - Don't comment on formatting, linting, or type errors
3. Be specific - Reference file paths and line numbers
4. Be actionable - Provide concrete suggestions, not vague concerns
5. Be proportionate - Minor issues shouldn't block, but note them
6. Assume competence - The author knows C++; explain only non-obvious context
7. Permission to be quiet - If the PR has no meaningful issues, post a short LGTM (one or two sentences) and stop. Do not manufacture nitpicks to fill space — padding trains authors to ignore Claude reviews. A clean PR getting a clean and high-signal review is the correct outcome.

Files to Reference

When reviewing, consult these project files for context:

• CLAUDE.md - Coding style and project guidelines
• CODING_STYLE.md - Complete coding style guide