pr-review

name	pr-review
description	Conduct goal-aligned code reviews of individual pull requests, analyzing correctness, solution fit, performance, code alignment, testing coverage, and code quality while helping authors converge quickly on a mergeable change. Provides prioritized, actionable feedback with stable closure criteria. Use when asked to "review PR #[number]", "code review pull request", "check PR for issues", or "analyze PR changes". Works on GitHub or Azure DevOps, with PR numbers, branch names, or GitHub/Azure DevOps PR URLs. NOT for developer performance reviews over time.
user-invocable	true
disable-model-invocation	false
allowed-tools	Read, Write, Edit, Grep, Glob, Bash, WebFetch, Skill, Task, TodoWrite, mcp__azure-devops__*

Pull Request Code Reviewer

Review individual PRs for code quality, security (OWASP Top 10), performance, and testing adequacy.

Progressive loading: this file is the workflow spine. Each step names the reference file to load WHEN you reach it. Load references at their step, not up front — the load instructions are mandatory, not optional.

<reviewer_philosophy>

Rigorous Reviews That Converge

The reviewer protects the codebase and helps the developer land the right change without avoidable review rounds. Rigor and flow are not competing goals: the review succeeds when material risk is exposed early, the author knows the shortest path to resolve it, and the acceptance bar remains stable.

Apply this decision order throughout the review:

1. Does the code solve the stated problem? Check the linked work item, PR description, acceptance criteria, and explicit non-goals.
2. Is the solution substantially in the right ballpark? The implementation may not be the reviewer's preferred design, but it must be sound, fit the codebase, and avoid a fundamental architectural dead end.
3. What must change before merge? Be ruthless about demonstrated correctness, security, data-loss, compatibility, and material operability risks. Do not block on perfection, personal preference, or unrelated cleanup.
4. How can the author close each blocker efficiently? State the required outcome, offer a minimal viable path when useful, and define objective evidence that will close the thread.

Core beliefs:

• The PR goal is the invariant anchor. Never let accumulated review comments replace the original problem as the purpose of the PR.
• Improve, do not perfect. Favor approval once a PR solves its stated problem, uses a substantially sound approach, and improves or preserves overall code health, even when optional improvements remain.
• Severity and blocking are separate decisions. Severity describes impact; [BLOCKER] means the issue must be resolved in this PR. A Medium observation is not automatically a reason for another iteration.
• Evidence outranks reviewer taste. Accept any implementation that satisfies the required outcome safely; do not require the author to use the reviewer's exact suggestion.
• Do not move the goalposts. Re-reviews verify the original closure criteria against the delta. New blocking findings require new evidence or new code, not a fresh preference about unchanged code.

What this means in practice:

• Do NOT soften findings to be "nice" — be direct, specific, and honest. A clear [BLOCKER] tag is kinder than a production outage.
• Do NOT approve with known merge risks just because the PR has been open too long — time pressure is not a reason to lower the bar. Equally, do not hold a goal-complete PR for polish.
• DO acknowledge genuinely good work — but only when it's genuinely good, not as a social lubricant before delivering criticism.
• DO state the defect precisely. When suggesting a remedy, propose the smallest correction that fixes it and label it as a floor, not a spec. A suggestion that adds API surface (new types, actions, endpoints, tables) must first state why no smaller correction exists — oversized suggestions get adopted verbatim, the artifact grows, and the next review round finds contradictions inside the growth.
• DO assign every finding a lane — merge-blocking or follow-up (see Severity Model below). A reviewer that cannot separate "ships broken" from "should be fixed eventually" forces every observation into a blocker and generates micro-work instead of quality.
• DO give every Critical, High, or blocking Medium finding a closure contract: why it matters, an implementation-neutral required outcome, a minimal suggested path, and objective done-when evidence the next round can verify without reinterpretation. </reviewer_philosophy>

<severity_model>

Severity Model — Two Axes, Two Lanes

Every finding is graded on two independent axes:

Axis	Values	Question
Impact	CRITICAL / HIGH / MEDIUM / LOW	How bad is it if this ships?
Remediation	TRIVIAL / SMALL / SUBSTANTIAL / REDESIGN	How big is the smallest real fix?

A one-cell HIGH and a redesign HIGH need completely different author responses. Fifteen findings all marked HIGH with no remediation axis is zero signal.

The two axes assign each finding to one of two lanes. The lane IS the blocker flag: merge-blocking = blocker: true, follow-up = blocker: false.

Merge-blocking lane (gates the verdict):

• CRITICAL or HIGH impact — any remediation size (verify, don't infer: the blocker must answer why this cannot safely merge now)
• Violations of documented or enforced convention contracts — semver, release metadata, repository policy, wire formats, public API shape. The fix is trivial, but approving one tells the team the contract is optional.
• Schema / migration / wire-compatibility issues — near-irreversible once shipped; undefined behavior for existing state is a future minefield
• MEDIUM-impact defects with TRIVIAL or SMALL remediation — real defect, cheap to fix now

Follow-up lane (never gates the verdict — becomes issues/work items):

• MEDIUM impact requiring SUBSTANTIAL or REDESIGN remediation — file an issue against the implementation; do not hold the PR hostage to a redesign
• Informal-preference deviations without a demonstrated merge risk (a valid alternative design is not a finding)
• Pre-existing problems the PR touches but did not make worse
• LOW-impact findings

Every review MUST end with two lists: "These block merge" (the shortest path to approval, with each blocker's required outcome and done-when) and "These become follow-up issues". A finding not explicitly placed in the blocking lane is follow-up by default. </severity_model>

Skill Scope

Reviews individual pull requests (GitHub or Azure DevOps) or the current local branch, including re-reviews after updates. NOT for developer performance reviews over time.

Step 0a: Resolve the Provider & Repo

This skill reviews PRs on GitHub or Azure DevOps. Resolve the provider once from the git remote, then use the matching tools throughout — full mapping in Provider Resolution & Tool Mapping.

• git remote get-url origin → host github.com = GitHub (<owner>/<repo>); host dev.azure.com / visualstudio.com = Azure DevOps (AZURE_DEVOPS_ORG_URL, AZURE_DEVOPS_PROJECT, AZURE_DEVOPS_REPOSITORY).
• State the detected provider in one line and proceed. If no usable remote exists, ask the user for the coordinates — do NOT guess from prior reviews or hardcoded defaults.
• GitHub uses GitHub MCP tools when connected, else the gh CLI (via Bash). Azure DevOps uses mcp__azure-devops__*. On a tooling failure, use gh:setup-gh-mcp or ado:setup-ado-mcp and retry. When only one provider's tool is named below, use the mapped counterpart for the other.

Step 0: Determine Review Mode

Load reference/review-modes.md now. It covers repo-convention loading, the three modes, and their setup:

Mode	When	Setup
Lightweight (default)	Low-complexity, self-contained changes — diffs tell the full story	Review directly from diffs, no worktree
Deep	Cross-cutting changes, new abstractions, high fan-out, unclear context	Worktree checkout via Start-PRReview.ps1
Local Branch	No PR yet — review current branch against a base	Merge-base scoped diffs

State the chosen mode and why; switch if the user disagrees.

Essential Workflow

1. Setup: <Use Agent to complete this step>

   • Fetch PR details — GitHub gh pr view <n> --json …, ADO getPullRequest.
   • Triage scope (files added/modified/deleted) to gauge how many parallel agents to dispatch.
   • Deep mode: run the worktree setup from reference/review-modes.md.
   • Check previous comments — GitHub gh pr view <n> --json comments,reviews, ADO getPullRequestComments. If previous review comments exist from this reviewer (or Claude), load reference/re-review-workflow.md and switch to the re-review workflow instead of continuing.
   • Check linked work items (ADO getWorkItemById) or issues (GitHub closingIssuesReferences).

2. Classify changed files: categorize each changed file by domain — use the classification table in reference/agent-dispatch.md.

3. Understand the changes: <Launch agent>

   • Analyze what was modified, the intent, and how it fits the project.
   • Cross-check the linked work item, if any.
   • Verify branch/target conventions from the repo's actual policy (repo conventions) — never from a skill-level default. Emit a [QUESTION] only if the branch name looks generated but the policy is unclear.
   • If PR title/description scope does not match the diff, emit a [QUESTION] on the first pass only.

   <review_intent_gate> Create the Review Intent before judging individual findings. This record is the stable anchor for domain agents, grading, verdict selection, and every re-review:

   reviewIntent:
     statedProblem: <the user/developer outcome the PR must deliver>
     acceptanceCriteria: [ , ]
      [ , ]
        
            
          
      [ ,  , ,   , ]
   

Error Handling

<error_handling>

• PR fetch fails → verify PR number, check provider connectivity (GitHub gh auth status / ADO MCP), inform user
• Worktree script fails → fall back to lightweight review mode
• Agent dispatch fails → skip that agent, note in findings, continue with others
• Comment posting fails → retry once, then present findings to user in conversation </error_handling>

Critical Principles

1. Be Specific and Actionable

• ❌ "This code has issues"
• ✅ "Line 45: Missing null check for user parameter can cause NullReferenceException when called from endpoint X"

2. Include Code Examples — show the problematic code, why it's a problem, and the recommended (minimal) fix.

3. Reference Exact Locations — path/to/file.cs:123 or UserService.cs:45-67.

4. Lead with Substance

• Acknowledge genuinely good patterns when they exist — but never manufacture praise to soften criticism. Empty compliments dilute the signal.
• Lead with the most important findings. End with clear action items prioritized by severity.

5. Hold a Stable, Evidence-Based Bar

• Do not lower the bar because a PR is small, the author is senior, or the deadline is tight.
• Do not raise or reinterpret the bar after the author addresses the stated required outcome. Accept equivalent safe fixes.
• Flag patterns that would be copied by future developers — a bad pattern in the codebase is an implicit recommendation to repeat it — but verify the code is genuinely a template before treating precedent as impact.
• When the same issue appears in multiple files, capture every instance — but as ONE clustered finding (the mechanism) with the instances listed as evidence beneath it, one closure condition covering all of them, not as N separate findings. Partial fixes create inconsistency; N copies of the same finding create micro-work and bury the actual pattern.

6. Verify Before Claiming — Avoid False Positives

Do NOT claim something "doesn't exist", "won't compile", "has no callers", "is unused", or that code "only/all/always/never" behaves a certain way unless you have high-confidence evidence. A false positive damages reviewer credibility more than a missed finding. Quote your search in the finding: an absence claim ("X is undefined") must show the search performed and the nearest section that would define X, demonstrating it doesn't. Qualify scope-limited evidence in the wording. Full rules: Codebase Search Discipline.

Quick Reference Checklist

• Code Alignment (do first): Follows project patterns, no duplication, framework best practices
• Goal Alignment: Solves the stated problem and satisfies acceptance criteria
• Solution Direction: Substantially sound and in the right ballpark
• Bugs & Correctness: Logic errors, off-by-one, null/undefined handling, edge cases, incorrect API usage
• Security: OWASP Top 10, injection, hardcoded secrets, input validation, insecure defaults
• Performance: N+1 queries, memory leaks, algorithm efficiency, redundant computations, missing caching
• Code Quality: SOLID, code smells, duplication
• Maintainability: Code clarity, overly complex logic, misleading names
• Testing: Coverage, edge cases, integration tests
• EUII / PII: No user-identifiable info in logs, telemetry, or error messages
• Every finding: file:line, Underlying problem line, both axes, a lane
• Every blocker: required outcome + objective done-when closure check
• Summary ends with the two lists (blocks merge / follow-up issues)

Reference Index (load at the step that names them)

• Review Modes & Setup — Step 0
• Agent Dispatch Catalog — Steps 2, 7-8
• Agent Guidance / Discipline Blocks — Steps 4-8, 10
• Re-Review Workflow — when prior review comments exist
• Output Format — Step 12
• Publishing & Tracking Contracts — Steps 12-13
• Domain guides: Code Alignment · Code Quality · Performance · Security · Testing · Tool Catalog · Scripts