Review Pr

Universal PR Review Workflow

Use this workflow to compare a source (PR) branch against a target branch, perform a code review following clean code, SOLID, and testing guidelines, and format the feedback for the author — all without disturbing the user's current checkout.

Modes

Usage: review-pr [pr-number|branch] [simple|advanced]

Default is simple when no mode is given. The words "advanced", "deep", or "thorough" in the user's request select advanced.

Steps tagged [advanced] below are skipped entirely in simple mode; untagged steps run in both modes.

Steps

1. Resolve Inputs

Determine the source branch, target branch, and (if available) PR context:

• If given a PR/MR number and a forge CLI is available, resolve the branches and pull the PR title/description as reviewer context:

  gh pr view <n> --json headRefName,baseRefName,title,body   # GitHub
  glab mr view <n>                                           # GitLab
  

  On Bitbucket, use the bb CLI or the REST API if configured.
• Pure-git fallback: the user supplies the source/target branch names. If no target is given, detect the default branch:

  git symbolic-ref refs/remotes/origin/HEAD   # if unset: git remote set-head origin --auto
  

• Forge CLIs are optional everywhere in this workflow: detect availability (e.g. command -v gh), use them when present, and fall back to pure git silently.

2. Update Repository

Fetch all remote branches and prune deleted ones to ensure you are comparing the latest commits:

git fetch --all --prune

3. Triage the Change

Get the size, shape, and intent of the change before reading any code:

git diff --stat origin/<target_branch>...origin/<source_branch>
git diff --name-only origin/<target_branch>...origin/<source_branch>
git log origin/<target_branch>...origin/<source_branch> --oneline

The commit log reveals the developer's intent and context. For large PRs (roughly >15 files or >600 changed lines), group the files by module/area and review chunk by chunk in the later steps instead of one monolithic pass. Never silently skip files — if anything is left unreviewed, say so in the report.

4. [advanced] Fetch Existing Review Discussion

Forge only; skip silently in pure-git mode. Purpose: never repeat feedback other reviewers already gave.

• List existing review comments and threads, including resolved ones:

  gh pr view <n> --comments                                  # GitHub, thread overview
  gh api repos/{owner}/{repo}/pulls/<n>/comments             # GitHub, file/line anchors + comment ids
  glab mr view <n> --comments                                # GitLab, thread overview
  glab api projects/:id/merge_requests/<n>/notes             # GitLab, note ids
  

  On Bitbucket, use REST .../pullrequests/<n>/comments.
• Keep a working summary: what was raised, by whom, at which file:line, and whether it is resolved. It is used for dedup in step 11 and for the reply/reaction sections in steps 12–13.

5. Inspect the Diff

Examine the exact changes introduced in the source branch:

git diff origin/<target_branch>...origin/<source_branch>
git diff origin/<target_branch>...origin/<source_branch> -- <file_path>   # single file

Apply two review disciplines:

• Read the surrounding context of each changed region (the whole function/class, not just the hunk) before judging it — git show origin/<source_branch>:<file_path> reads a file without checking it out.
• Distinguish issues introduced by this PR from pre-existing ones. Only the former become findings; a serious pre-existing issue may be mentioned once, clearly labeled as out of scope.

6. Verify Architectural & Project-Specific Compliance

Before looking at code quality, check the project's own instructions (e.g., AGENTS.md, CLAUDE.md, .cursorrules, or .agent/rules/ directory):

• Layer Separation: Ensure strict separation of concerns (e.g., presentation/UI layers must never access database/API/repository classes directly).
• Framework Leakage: Keep business/domain logic free of UI framework concepts (e.g., ViewModels/controllers must not reference Context, Activity, Fragment, or framework views).
• Dependency Injection (DI): Verify new dependencies are registered correctly using the project's preferred DSL and scope (e.g., lazy/factory vs singleton).
• Type-Safe Seams: Check that routes, navigation parameters, and events use type-safe declarations instead of loose strings.
• Component Design: UI elements should accept layout/style properties (like Modifier in Compose or classes in Web) as optional arguments to promote reuse.

7. Verify Clean Code & SOLID Compliance (Clean Code Guard)

Ensure the changed production code adheres to clean code and SOLID design principles in any programming language:

• Names Reveal Intent: Never use generic names like data, result, temp, item, or generic verbs/nouns (do_process, helper, manager) without qualifiers.
• Functions Stay Small: Target ≤ 20 lines of code, doing exactly one thing at a single level of abstraction.
• Parameter Ceiling: Maximum of 4 arguments. For 5+ arguments, introduce a request/config DTO object. Never use boolean flag parameters (split into two functions instead).
• Command/Query Separation: A function either returns a value (query) or has a side effect (command), never both.
• Comments & Structure: Comments explain why, not what. Delete commented-out code, unused imports, and left-over TODO comments (version control keeps history).
• Single Responsibility (SRP): A module/class should have only one reason to change (be answerable to exactly one actor).
• Open/Closed (OCP): Prefer extending behavior by adding new code (using interfaces, strategy patterns, or registries) instead of modifying existing conditional branches (like if-else or switch).
• Liskov Substitution (LSP): Subclasses or implementations must fulfill the parent contract. Never override a method to throw UnsupportedOperationException or bypass a contract.
• Dependency Inversion (DIP): Abstractions (interfaces) belong with the client package that consumes them, not next to the concrete implementation.
• DRY & YAGNI: Eliminate duplicate business knowledge. Never write speculative code (no unused parameters, flags, configs, or "just in case" exports).
• Complexity Ceiling: Cyclomatic complexity ≤ 10 and nesting depth ≤ 5 per function; request a refactor before exceeding.
• Dead Code: Flag unused imports, unused symbols, and unreachable branches introduced by the PR.
• AI-specific Safeguards:
  • Never swallow errors with broad catch-all handling (empty catch blocks are banned).
  • Do not add defensive null/type checks for cases already guaranteed by the type system.
  • Never return hardcoded mock/fake data from real production methods.

8. Verify Test Code Quality (Test Guard)

If the PR contains new or modified tests, verify they follow best practices:

• Test Behavior, Not Implementation: Assert return values and state changes visible to the caller. Do not assert that internal helper functions were called with specific arguments (brittle mocks).
• Justify Mocks: Mock only at system boundaries (network APIs, databases, filesystem, clock, random, third-party SDKs). Never mock internal helpers, DTOs, or domain entities.
• Data-Driven Variants: Combine tests with identical setups but different values into a single parameterized test.
• Scenario Naming: Name tests clearly for the scenario and expected outcome: test_<scenario>_<expected_outcome>.
• Infrastructure Subject: When database queries, schemas, or persistence logic is the subject of the test, run it against a real test database, not a mock.
• Missing Tests: Changed or new behavior with no covering test is a finding in itself — check test existence, not only test quality.
• Regression Tests Are Sacred: A PR must never delete or weaken a test that references a production issue/incident ID.

9. [advanced] Cross-Cutting Review Dimensions

• Security: No secrets/credentials in the diff; injection risks (SQL/command/path); authorization checks on new endpoints/actions; unsafe handling of untrusted input at trust boundaries.
• Error Handling & Edge Cases: Failure paths of new code (null/empty, timeouts, partial results); errors surfaced, not swallowed.
• Performance: N+1 queries, work inside loops or hot paths, unnecessary allocations, missing pagination on unbounded data.
• Dependency Changes: Every new dependency justified; lockfile consistent with the manifest; no unexpected major-version bumps.
• Compatibility: API contract changes, DB schema/migrations (backward + forward safe), serialized formats, feature flags.

10. [advanced] Optional Verification via Worktree

Only if a local build is feasible and the user wants it — CI owns build/test otherwise; skip gracefully when the project is not runnable locally.

git worktree add <tmpdir> origin/<source_branch>   # never disturb the user's checkout

• Discover the project's own commands from CI config or package scripts (e.g. .github/workflows/, package.json scripts, Makefile, gradlew tasks); run the test suite and linters/static analysis.
• Always clean up: git worktree remove <tmpdir> (use --force only if the build dirtied it).

11. Verify Findings Before Reporting

Anti-hallucination discipline, both modes:

• For each candidate finding, re-check that the exact file and line exist in the diff and that the PR introduced the issue. Drop anything you cannot confirm by re-reading the code.
• Review only changed lines and their blast radius. Never demand refactors of untouched code.
• [advanced] Dedup against existing comments: A finding already raised by another reviewer (step 4) is NOT repeated. Move it to the "already raised" bucket — agree, add nuance, or suggest a reaction instead (steps 12–13).

12. Format the Review Output

When presenting the review report:

1. Explain it for a dummy first: Start with a high-level, simplified summary of what the PR does and how it achieves its goal.
2. Review as the reviewer: Evaluate the code professionally, focusing on architecture, code quality, and maintainability.
3. Severity on every finding, ordered most severe first: 🔴 blocker · 🟠 major · 🟡 minor · 🔵 nit.
4. Overall verdict: approve / approve with nits / request changes.
5. Format comments for easy copy-pasting: display the file name and line number for each comment, and place the suggested change or feedback inside a copy-pasteable markdown block. When the fix is a small concrete change on GitHub, put it in a ```suggestion block so the author can apply it in one click.
6. Consolidate repetitive comments: If an issue occurs multiple times, write a single comment at the first occurrence, list all other file paths and line numbers where it occurs, and request a global fix.

Fill in this template:

## Summary
<what the PR does, in plain words>

## Verdict
<approve | approve with nits | request changes>

## Findings
### 🔴 <title> — `file:line`
<comment; optional ```suggestion block>
<!-- repeat per finding, ordered by severity -->

[advanced] the template gains two more sections:

## Already raised by others
- <quote/link existing comment> → ready-to-post reply: "<agree or add nuance>"

## Suggested reactions
- <existing comment> → 👍

13. [advanced] Post Replies & Reactions (Gated)

Forge only. NEVER auto-post. Present the proposed replies/reactions from step 12 and post only on explicit user approval:

gh api repos/{owner}/{repo}/pulls/<n>/comments/<comment_id>/replies -f body='...'   # GitHub reply
gh pr comment <n> --body '...'                                                     # GitHub top-level comment
gh api -X POST repos/{owner}/{repo}/pulls/comments/<comment_id>/reactions -f content='+1'   # GitHub reaction
glab mr note <n> -m '...'                                                           # GitLab reply
glab api -X POST projects/:id/merge_requests/<n>/notes/<note_id>/award_emoji -f name=thumbsup   # GitLab award

Pure-git mode: this step does not apply; everything stays copy-pasteable output.