github-pr-review

name	github-pr-review
description	Qualified, critical code reviews using GitHub's pending review API. Inline comments, verification, and validation.
metadata	{"category":"engineering"}

GitHub PR Review Skill 🔍

Qualified, critical code reviews using GitHub's pending review API.

Use this skill when reviewing pull requests. Produces inline code comments, code suggestions, and a qualified verdict (APPROVE / REQUEST_CHANGES / COMMENT). Always validates that changes match the PR description before assessing quality.

Philosophy

• Be critical. Actively look for problems -- don't rubber-stamp.
• Validate claims. Does the code actually do what the PR description claims?
• Verify impact. Are there side effects, missing edge cases, security issues?
• Inline findings. Every issue goes directly on the affected line, not just in a summary.
• One review, one notification. Always batch via pending reviews.

Trigger Modes

This skill activates in one of these ways:

Mode	How
Manual	User says "review PR #42" or links a PR
Scheduled	Cron job polls for open PRs without a review yet
@mention	Agent is tagged in a PR comment or requested as reviewer

# Example: find PRs awaiting review (for scheduled mode)
gh pr list --repo <owner>/<repo> --json number,title,reviewDecision \
  --jq '.[] | select(.reviewDecision == "" or .reviewDecision == "REVIEW_REQUIRED")'

Prerequisites

• gh CLI installed and authenticated (gh auth status)
• Collaborator / write access on the target repo

Configuration

Adjust review depth based on model capabilities and budget:

Scenario	Recommended Model	Notes
Security / complex logic	Opus or Sonnet	Worth the tokens for critical paths
Standard feature PRs	Sonnet	Good balance of depth and cost
Style / docs / config	Haiku or Flash	Fast triage, low cost
Large diffs (>500 lines)	Split approach	Haiku for triage, Sonnet for flagged files

Review Workflow

Step 1: Gather Context

REPO="owner/repo"
PR=42

# PR metadata
gh pr view $PR --repo $REPO --json title,body,state,baseRefName,headRefName,commits,files

# Full diff
gh pr diff $PR --repo $REPO

# Latest commit SHA (needed for review API)
COMMIT=$(gh pr view $PR --repo $REPO --json commits --jq '.commits[-1].oid')

Step 2: PR Hygiene Check

Before reviewing code, validate the PR itself:

Title & Description:

• Does the title follow conventional commits? (feat: / fix: / chore: / docs:)
• Does the description explain Why (context), What (changes), and Impact (outcome)?
• Are there changes NOT mentioned in the description? (scope creep)
• Are there claims in the description NOT reflected in the code? (incomplete)

Metadata:

• Is someone assigned?
• Are appropriate labels set? (enhancement, bug, documentation, etc.)
• Is the base branch correct? (usually main)

Scope:

• Is this a single focused change, or does it mix unrelated things?
• Could this be split into smaller PRs?

If any of these are missing or wrong, flag it in the review. PR hygiene is not optional -- sloppy PRs lead to sloppy repos.

Step 3: Analyze the Diff

Read every changed file. Prioritize checks by severity:

🔴 Always check (blocking):

Check	What to Look For
Correctness	Logic errors, off-by-ones, wrong conditions
Security	Injection, secrets in code, unsafe deserialization, auth gaps
Secrets in diffs	High-entropy strings (>20 chars, Base62/Base64/hex) in code, docs, examples, configs, markdown. If it looks like a real token and isn't an obvious placeholder (your-token-here, xxx, <token>, sk-...), it's a blocking finding -- even in documentation
Breaking changes	API contract changes, removed fields, changed defaults

🟡 Check if relevant (important):

Check	What to Look For
Error handling	Missing try/catch, unchecked return values, silent failures
Edge cases	Null/undefined, empty arrays, boundary values
Tests	Missing test coverage for new/changed behavior
Performance	N+1 queries, unnecessary allocations, missing indexes

🟢 Nice to have (non-blocking):

Check	What to Look For
Dead code	Unused imports, unreachable branches, leftover debug code
Naming & clarity	Misleading names, magic numbers, missing comments on complex logic

Step 4: Create Pending Review with Inline Comments

Always use the 2-step pending review pattern -- even for a single comment.

# Create pending review with inline comments
gh api repos/$REPO/pulls/$PR/reviews \
  -X POST \
  -f commit_id="$COMMIT" \
  -f 'comments[][path]=src/auth.ts' \
  -F 'comments[][line]=25' \
  -f 'comments[][side]=RIGHT' \
  -f 'comments[][body]=Missing null check -- `user` can be undefined when token is expired.

```suggestion
if (!user) {
  throw new AuthError("Invalid or expired token");
}
```' \
  -f 'comments[][path]=src/auth.ts' \
  -F 'comments[][line]=40' \
  -f 'comments[][side]=RIGHT' \
  -f 'comments[][body]=This catches all errors silently. At minimum, log the error.' \
  --jq '{id, state}'

# Returns: {"id": 12345, "state": "PENDING"}

Parameter reference:

Parameter	Flag	Notes
commit_id	-f	Latest commit SHA from Step 1
comments[][path]	-f	File path relative to repo root
comments[][line]	-F	Line number (end line for multi-line) -- numeric, use -F
comments[][side]	-f	RIGHT for added/modified lines, LEFT for deleted lines
comments[][body]	-f	Comment text, optionally with ```suggestion block
comments[][start_line]	-F	For multi-line suggestions (optional)

Syntax rules:

• Always single-quote 'comments[][...]' parameters
• -f for strings, -F for numbers
• Code suggestions replace the entire specified line range

Step 5: Submit the Review

REVIEW_ID=12345  # from Step 3 response

gh api repos/$REPO/pulls/$PR/reviews/$REVIEW_ID/events \
  -X POST \
  -f event="REQUEST_CHANGES" \
  -f body="## Review Summary

Found 2 issues:
1. Missing null check in auth flow (blocking)
2. Silent error swallowing (blocking)

Please address before merge."

Step 6: Post-Review Action

After submitting:

Verdict	Action
APPROVE	Merge if auto-merge is enabled/permitted for the repo: gh pr merge $PR --repo $REPO --squash --delete-branch. Otherwise leave approval for a human to merge.
REQUEST_CHANGES	Done. Author fixes, then re-review.
COMMENT	Done. No blocking action needed.

⚠️ Auto-merge is opt-in per repo/team. Don't auto-merge unless explicitly configured. Check your agent's operational rules (MEMORY.md / AGENTS.md) for repo-specific merge policies.

Step 7: Handle Author Responses

After submitting a review, the PR author may reply to your comments. Follow up:

If the author pushed fixes:

• Re-review the new diff (gh pr diff) -- don't just trust "fixed"
• Reply to your original comments confirming the fix or flagging remaining issues
• Submit a new review with updated verdict

If the author disputes a finding:

• Read their argument carefully -- you might be wrong
• Reply directly on the comment thread (not in a new review)
• If they're right: acknowledge it and resolve
• If you still disagree: explain why with specific reasoning

If the author asks a question:

• Answer directly on the comment thread
• If it reveals you made a wrong assumption, update your review

# Reply to a review comment thread
gh api repos/$REPO/pulls/$PR/comments/<COMMENT_ID>/replies \
  -X POST -f body="Good point -- I missed that the null check happens upstream. Withdrawing this finding."

After confirming a fix: Resolve the conversation thread so the PR shows a clean state before merge.

# List open review threads to find IDs
gh api graphql -f query='
  query {
    repository(owner: "<owner>", name: "<repo>") {
      pullRequest(number: <PR>) {
        reviewThreads(first: 50) {
          nodes { id isResolved comments(first: 1) { nodes { body } } }
        }
      }
    }
  }
' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false)'

# Resolve a confirmed thread
gh api graphql -f query='
  mutation {
    resolveReviewThread(input: {threadId: "<THREAD_NODE_ID>"}) {
      thread { isResolved }
    }
  }
'

Event Type Decision Guide

Is there a bug, security issue, or broken behavior?
  → YES → REQUEST_CHANGES

Is something missing (tests, error handling, docs)?
  → YES → REQUEST_CHANGES

Are there only style/naming/optional improvements?
  → YES → APPROVE (with suggestions as non-blocking comments)

Just questions or neutral observations?
  → COMMENT

Review Body Structure

The overall review body (Step 5) should follow this template:

## ✅ / ❌ / 💬 Code Review

**Verdict: APPROVE / REQUEST_CHANGES / COMMENT**

### Validation
- Does the code match the PR description?
- Are the changes correct and complete?
- Are there changes NOT mentioned in the PR description? (scope creep)
- Are there claims in the description NOT reflected in the code? (incomplete)

### Verification
- Side effects? Breaking changes? Missing tests?
- Security or performance concerns?

### Risk Assessment
- Low / Medium / High -- with reasoning

Code Suggestions

Use GitHub's suggestion syntax for concrete fixes:

This variable name is misleading -- it holds a list, not a single item.

```suggestion
const users = await fetchUsers();
```

Clear naming helps the next reader.

Multi-line suggestions use start_line:

-F 'comments[][start_line]=10' \
-F 'comments[][line]=15' \

This replaces lines 10–15 with the suggestion content.

Common Pitfalls

Pitfall	Fix
Posting comments one-by-one (notification spam)	Always use pending review → submit
Approving without reading the diff	Read every changed line
Only reviewing the "interesting" files	Check configs, tests, manifests too
Vague comments ("this looks off")	Be specific: what's wrong, why, and how to fix
Approving with blocking issues as "suggestions"	If it must be fixed → REQUEST_CHANGES
Using inline \n in gh pr create --body	Use heredoc or temp file for multi-line bodies
Auto-merging on repos without permission	Check merge policy before merging
"Docs-only = zero risk" bias	Security checks apply to every line in the diff, including docs, examples, and markdown. A real token in a code example is just as leaked as one in source code
Approving real secrets in examples	Any high-entropy string in a diff that isn't an obvious placeholder (your-token-here, <TOKEN>, xxx) must be flagged as a potential leaked secret -- especially when the PR is about auth/token usage

Secret Detection in Diffs

Every diff line must be scanned for potential secrets -- including docs, examples, and markdown.

Heuristics

A string is suspicious if:

1. Length ≥ 20 characters and high entropy (mixed case, digits, no dictionary words)
2. Looks like a token format: Base62, Base64, hex, sk-..., tok-..., ghp_..., clw_...
3. Not an obvious placeholder: your-token-here, xxx, <TOKEN>, example-key, sk-...your-key...
4. Context is auth-related: the PR touches auth docs, token handling, API key usage, or .env examples

What to do

• If suspicious → REQUEST_CHANGES with: "This looks like a real secret. Please replace with a placeholder."
• If the author confirms it's a dummy → they can re-push with a comment explaining why
• Never approve a PR with a suspicious high-entropy string without explicitly verifying it's not real

Elevated sensitivity triggers

When a PR is about auth, tokens, API keys, or secrets handling, assume every token-shaped string is real until proven otherwise. The subject matter itself is the red flag.

Real-world example (incident)

A docs PR added a callout explaining correct X-API-Key usage. The example contained a real agent token (hlFPU...32chars). It was approved and merged because the reviewer classified it as "docs-only, zero risk." The token had to be rotated.

Lesson: "Docs-only" is never "zero risk" when the diff contains strings that look like secrets.

Review Without Inline Comments

For clean PRs with no issues, skip the 2-step pattern:

gh api repos/$REPO/pulls/$PR/reviews \
  -X POST \
  -f commit_id="$COMMIT" \
  -f event="APPROVE" \
  -f body="## ✅ Code Review

**Verdict: APPROVE**

Reviewed all changed files. Clean removal of dead code, no side effects, no orphaned references.

**Risk: Zero.**"

---

Part of the agentic-foundation skill library.