Revue de PR GitHub avec gh CLI

---

name: pr-review allowed-tools: Bash(gh:), Bash(git clone:), Bash(git log:), Bash(git diff:), Bash(mkdir:), Bash(grep:), Bash(find:), Bash(wc:), Bash(jq:*), Read, Write description: Comprehensive GitHub PR code review — fetches diff/metadata/comments via gh CLI, writes review files, posts only on /send or /send-decline version: "1.8.0"

Overview

This is a two-stage PR review. All analysis happens locally first. Nothing is posted to GitHub until you run /send or /send-decline.

---

Step 1 — Parse PR Reference

$ARGUMENTS will be one of:

• A full GitHub PR URL: https://github.com/owner/repo/pull/123
• A PR number with repo: owner/repo 123 or owner/repo#123
• A PR number alone (uses the current repo): 123

Parse $ARGUMENTS to extract:

• REPO — e.g. ddclient/ddclient
• PR_NUM — numeric PR number
• REVIEW_DIR — /tmp/pr-review/${REPO}/${PR_NUM}

If $ARGUMENTS is empty or cannot be parsed, ask the user for the PR URL or number before continuing.

---

Step 2 — Fetch PR Data

Run each command below. Create the workspace first:

mkdir -p "/tmp/pr-review/${REPO}/${PR_NUM}/pr"

PR metadata (one call, captures all needed fields):

gh pr view ${PR_NUM} --repo ${REPO} \
  --json number,title,body,author,state,headRefName,baseRefName,additions,deletions,files,commits,labels,assignees,reviewRequests \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/metadata.json"

Diff:

gh pr diff ${PR_NUM} --repo ${REPO} \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/diff.patch"

Inline code review comments (single call with --jq):

gh api "/repos/${REPO}/pulls/${PR_NUM}/comments" \
  --jq '[.[] | {path,line,body,user_login:.user.login,created_at}]' \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/inline_comments.json"

PR review summaries (approvals, request-changes, etc.):

gh api "/repos/${REPO}/pulls/${PR_NUM}/reviews" \
  --jq '[.[] | {user_login:.user.login,state,body,submitted_at}]' \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/reviews.json"

General issue/PR comments:

gh api "/repos/${REPO}/issues/${PR_NUM}/comments" \
  --jq '[.[] | {user_login:.user.login,body,created_at}]' \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/issue_comments.json"

Latest commit SHA (needed for inline comment posting later):

gh pr view ${PR_NUM} --repo ${REPO} \
  --json commits --jq '.commits[-1].oid' \
  > "/tmp/pr-review/${REPO}/${PR_NUM}/head_sha.txt"

Read and reason over each fetched file before proceeding.

---

Step 3 — Analyze

Review the fetched data against all categories below. Document findings as you go — you will write them to files in Step 4.

Anti-Rationalizations

Before starting, reject these common shortcuts:

| Rationalization | Why it's wrong | |---|---| | "Small PR, quick skim" | Heartbleed was two lines. Classify by risk, not size. | | "Just a refactor, no security impact" | Refactors break invariants and silently change failure modes. Treat as unknown until proven low-risk. | | "The tests pass so it's fine" | Tests can be tautological, mock the wrong symbol, or cover only the happy path. | | "I know this codebase well" | Familiarity creates blind spots. Follow the checklist anyway. | | "The author is experienced, it'll be fine" | Review the code, not the author's reputation. |

Review Categories

| Category | Key Questions | |---|---| | Functionality | Does the code solve the stated problem? Edge cases handled? Regressions possible? | | Correctness | Logic errors? Off-by-one? Null/undef dereferences? Guard clauses used to reduce nesting rather than deep if/else chains? What are the failure modes — do they fail loudly, silently, or dangerously? For changes to shared, base, or widely-used code: is the impact radius understood? Could this break callers in non-obvious ways? Concurrency: race conditions, shared mutable state, or missing locks in concurrent contexts? For bug fixes: does the fix validate at multiple layers (entry point, business logic, environment guard) or only patch the immediate call site? A single check can be bypassed by a different code path, a mock, or a refactor. When building protocol payloads (XML, JSON, URLs) through string concatenation or heredocs, check for stray trailing characters — an extra ", unclosed delimiter, or wrong quoting form — that make the payload syntactically invalid; the server typically returns a generic 400 error that is hard to diagnose in the field. | | Readability | Meaningful names? DRY? Comments explain why not what? Magic numbers replaced with named constants? Functions kept to a reasonable length, with abstraction applied where appropriate? Are there one-use trivial helpers or wrapper classes that add indirection without value? Is there a simpler built-in or standard library method that could replace custom logic? | | Style | Consistent with codebase conventions? Follows project patterns? | | Performance | Any O(n²) where O(n) would do? Unnecessary repeated calls? | | Security | Injection? Unvalidated external input? Credential exposure? Fail-closed: when an operation fails (network timeout, parse failure, API error), does the code deny/fail safely rather than silently proceed with stale or empty data? For PRs that add or update a dependency: is the version pinned, the source auditable, and are there no known CVEs? When a protocol or module constructs its own HTTP requests rather than delegating to a shared client, verify that caller-configured options (proxy, SSL settings, timeouts) are explicitly threaded through — omitting them means user configuration is silently bypassed, potentially leaking traffic or failing in restricted environments. | | Testing | Tests exist? Cover success and error paths? Assertions meaningful? Does the PR description show evidence the author ran and tested the change locally? Are any existing assertions weakened, removed, or replaced with tautologies to make tests pass? | | PR Quality | Focused scope? Commit messages clear? Description accurate? PR not in draft/WIP without proper designation? For user-facing changes, is a changelog or NEWS entry included? Is this change solving the right problem — not just implementing what was literally asked? For config file or API changes: is backward compatibility with existing deployments considered and are migration steps documented? Configuration key consistency: do examples, help text, or documentation reference config keys by their exact declared names? A mismatch (documenting region= when the code reads aws-region) silently misconfigures deployments with no error. Unused declared config variables: are there variables declared in a config registry, argparse definition, or help table that the implementation never reads? Dead declarations appear in --help output and mislead users into setting options that have no effect. |

Testing — additional checks:

• Does the stub or mock target the exact symbol production code calls? A stub wired to the wrong namespace or import path never fires (e.g., Perl: CORE::sleep vs ddclient::sleep; Python: the import path at call time, not the definition path).
• Would this test still fail if the production logic it targets were removed or broken? If not, the test provides no regression coverage.
• Are bare numeric literals in test assertions derived from configurable defaults? A default value is not a correctness invariant — tests should use the configurable parameter directly rather than hardcoding the default's computed value (e.g., asserting jitter is always < interval * 0.2 when 0.2 is just the default percentage, not a fixed bound).
• Does the test verify real behavior or mock/stub behavior? A test that asserts only on the presence of a mock marker (e.g., a -mock test ID, a stub return value, or a spy call count without checking what it was called with) tells you the mock fired — it says nothing about whether the code under test works correctly.
• Are mock objects structurally complete? Partial mocks that include only the fields the author expected to need can silently hide failures when downstream code accesses omitted fields. A mock should mirror the full shape of the real object.
• Are test-only utilities kept out of production classes? Methods added to production code solely for test setup or cleanup (e.g., a destroy() or reset() that nothing in production calls) pollute the interface, risk accidental invocation, and are a signal the design may need reconsideration.
• Does error-handling code fall back to a cached or default value silently on failure? A catch block that swallows an exception and returns stale or empty data looks like success to the caller but hides the underlying error. Fallbacks must be explicit, logged, and scoped to the expected failure type — broad catch blocks that fall back to defaults also mask unrelated errors.

Finding Priority Markers

• Blocker — must be fixed before merge
• Important — should be addressed
• Nit — minor style or preference
• Suggestion — consider for future
• Question — needs clarification from author
• Praise — acknowledge excellent work

Analysis Steps

1. Read metadata.json — understand scope, linked issues, existing reviews
2. Check for a project conventions file (CLAUDE.md, CONTRIBUTING.md, or similar) at the repo root and factor its rules into the review
3. Git history check — for each file touched by the PR, scan recent commits for removals of security-sensitive code:

   gh api "/repos/${REPO}/commits" --jq '.[].commit.message' | grep -iE 'fix|security|cve|auth|vuln' | head -20
   

   If the PR removes or rewrites code that was previously added by a commit mentioning fix, security, CVE, or auth, treat that as an immediate red flag requiring deeper scrutiny — the removed code may have been a deliberate safeguard.
4. Read the test changes in diff.patch first, before the implementation — weakened or removed assertions are easiest to spot in isolation, before the implementation anchors your expectations
5. Read the implementation changes in diff.patch — walk through every changed file
6. Read inline_comments.json and reviews.json — note what reviewers already flagged (avoid duplicating)
7. Read issue_comments.json — any discussion context
8. Recurring reviewer patterns — for the 1-2 most-modified files, check whether similar issues have been raised in previous PRs by finding recent PR numbers from the commit log:

   gh api "/repos/${REPO}/commits?path=PATH&per_page=10" \
     --jq '.[].commit.message' | grep -oE '#[0-9]+' | head -5
   

   For each PR number found, spot-check its review comments. If the author has been flagged for the same pattern before and it recurs here, that is worth noting — patterns matter more than isolated incidents.
9. Check commit messages for clarity and accuracy
10. Apply the category checklist above

---

Step 4 — Write Review Files

Write two files using the Write tool:

review.md — Internal detailed review

Path: /tmp/pr-review/${REPO}/${PR_NUM}/pr/review.md

Include:

• PR metadata summary (title, author, branch, size)
• Overall assessment
• Findings grouped by priority (Blocker → Important → Nit → Suggestion → Question → Praise)
• Each finding: category, file:line if applicable, issue, recommendation
• Proposed inline comments (file, line, side, comment body; for concrete code fixes include a ```suggestion block — renders as a one-click Apply button in GitHub)

Use whatever formatting is useful for your own analysis — this file is not posted.

human.md — Review text to post to GitHub

Path: /tmp/pr-review/${REPO}/${PR_NUM}/pr/human.md

Rules for this file:

• No emojis
• No em-dashes — use commas or semicolons instead
• No line number references (GitHub renders diffs, not line numbers in comment bodies)
• No internal analysis notes
• Direct, professional tone — consistent with project communication style
• Lead with genuine Praise before listing issues — accurate acknowledgement of what the author got right builds trust in the critical feedback that follows
• Then findings grouped by priority
• End with overall recommendation: approve / request changes / comment

---

Step 5 — Create /send and /send-decline Commands

Write these two command files so the user can post the review when ready.

~/.claude/commands/send.md — content:

Post the prepared review for PR #${PR_NUM} in ${REPO} as an **approval**.

1. Read the commit SHA from: /tmp/pr-review/${REPO}/${PR_NUM}/head_sha.txt
2. Read proposed inline comments from: /tmp/pr-review/${REPO}/${PR_NUM}/pr/review.md
3. Read the review body from: /tmp/pr-review/${REPO}/${PR_NUM}/pr/human.md

Step A — Create a PENDING review, bundling all proposed inline comments:

  gh api "/repos/${REPO}/pulls/${PR_NUM}/reviews" -X POST \
    -f commit_id="HEAD_SHA" \
    -f 'comments[][path]=PATH' -F 'comments[][line]=LINE' -f 'comments[][side]=RIGHT' -f 'comments[][body]=BODY' \
    --jq '{id,state}'

  Repeat the four comments[][] fields for each inline comment.
  Omit all comments[][] fields if there are no inline comments.
  For multi-line ranges: prepend -F 'comments[][start_line]=N' before the line field.
  Use side=LEFT for deleted lines, side=RIGHT for added or context lines.
  For concrete code fixes: include a ```suggestion block in the body.

Step B — Submit the pending review using the id returned from Step A:

  gh api "/repos/${REPO}/pulls/${PR_NUM}/reviews/REVIEW_ID/events" -X POST \
    -f event="APPROVE" \
    -f body="$(cat /tmp/pr-review/${REPO}/${PR_NUM}/pr/human.md)"

Confirm success and show the URL of the posted review.
Then delete ~/.claude/commands/send.md and ~/.claude/commands/send-decline.md so stale commands don't persist.

~/.claude/commands/send-decline.md — content:

Post the prepared review for PR #${PR_NUM} in ${REPO} as a **request for changes**.

1. Read the commit SHA from: /tmp/pr-review/${REPO}/${PR_NUM}/head_sha.txt
2. Read proposed inline comments from: /tmp/pr-review/${REPO}/${PR_NUM}/pr/review.md
3. Read the review body from: /tmp/pr-review/${REPO}/${PR_NUM}/pr/human.md

Step A — Create a PENDING review, bundling all proposed inline comments:

  gh api "/repos/${REPO}/pulls/${PR_NUM}/reviews" -X POST \
    -f commit_id="HEAD_SHA" \
    -f 'comments[][path]=PATH' -F 'comments[][line]=LINE' -f 'comments[][side]=RIGHT' -f 'comments[][body]=BODY' \
    --jq '{id,state}'

  Repeat the four comments[][] fields for each inline comment.
  Omit all comments[][] fields if there are no inline comments.
  For multi-line ranges: prepend -F 'comments[][start_line]=N' before the line field.
  Use side=LEFT for deleted lines, side=RIGHT for added or context lines.
  For concrete code fixes: include a ```suggestion block in the body.

Step B — Submit the pending review using the id returned from Step A:

  gh api "/repos/${REPO}/pulls/${PR_NUM}/reviews/REVIEW_ID/events" -X POST \
    -f event="REQUEST_CHANGES" \
    -f body="$(cat /tmp/pr-review/${REPO}/${PR_NUM}/pr/human.md)"

Confirm success and show the URL of the posted review.
Then delete ~/.claude/commands/send.md and ~/.claude/commands/send-decline.md so stale commands don't persist.

Replace ${PR_NUM} and ${REPO} with the actual values (not shell variables) when writing these files.

---

Step 6 — Present to User

Show the user:

1. A brief summary of findings (counts by priority level)
2. The path to review.md for detailed reading
3. The path to human.md for the draft post
4. Instructions: "Edit human.md if needed, then run /send to approve or /send-decline to request changes"

Do not post anything to GitHub until the user runs /send or /send-decline.

---

Inline Comment Syntax Reference

Inline comments are bundled into the PENDING review as part of /send and /send-decline (Step A). Field reference:

| Field | Flag | Notes | |---|---|---| | comments[][path] | -f | File path relative to repo root | | comments[][line] | -F | End line number (integer; -F for type coercion) | | comments[][start_line] | -F | Start line for multi-line ranges (omit for single-line) | | comments[][side] | -f | RIGHT for added/context lines; LEFT for deleted lines | | comments[][body] | -f | Comment text; see suggestion block format below |

Use -f for string fields, -F for integer fields (line, start_line).

Single-line comment:

-f 'comments[][path]=lib/ddclient.pm' \
-F 'comments[][line]=42' \
-f 'comments[][side]=RIGHT' \
-f 'comments[][body]=Comment text'

Multi-line comment (spans lines 40–42):

-f 'comments[][path]=lib/ddclient.pm' \
-F 'comments[][start_line]=40' \
-F 'comments[][line]=42' \
-f 'comments[][side]=RIGHT' \
-f 'comments[][body]=Comment text'

Code suggestion (renders as one-click Apply button in GitHub):

-f 'comments[][body]=Prefer int(rand($n)) to avoid silent float truncation passed to sleep:

```suggestion
my $jitter = int(rand($interval * $pct));
```'

---

References

Sources consulted when building and refining this checklist:

• SpillwaveSolutions/pr-reviewer-skill — original skill this was adapted from; gh CLI replacements for Python scripts applied during import https://github.com/SpillwaveSolutions/pr-reviewer-skill
• ParadiseSS13/Paradise discussion #21968 — community PR review checklist covering code cleanliness, readability, runtime prevention, and testing practices; BYOND-specific items excluded https://github.com/ParadiseSS13/Paradise/discussions/21968
• r/rails — "How do you approach PR reviews?" (u/arup_r, 2026) — practitioner discussion; contributed: test diff before implementation diff, tautological assertion detection, failure mode analysis, impact radius for shared code, unnecessary abstraction as tech debt, right-problem check
• bhserna review skill gist — Claude Code skill for branch/PR review; contributed: check project conventions file (CLAUDE.md/CONTRIBUTING.md) before reviewing, concurrency as an explicit correctness concern https://gist.github.com/bhserna/831bc50ad38378813eee9d9407609cf7
• ddclient/ddclient PR #888 review — real-world review where the testing depth checks were first identified: Perl namespace mismatch (CORE::sleep vs ddclient::sleep), regression sensitivity, and configurable-default-as-invariant in test assertions
• aidankinzett/claude-git-pr-skill — contributed: pending review API pattern (bundle inline comments into PENDING review before submitting), code suggestion ```suggestion block syntax, -f vs -F flag distinction for string/integer fields, multi-line inline comment ranges via start_line, LEFT/RIGHT side parameter https://github.com/aidankinzett/claude-git-pr-skill
• obra/superpowers — testing-anti-patterns — contributed: mock-behavior testing (tests that assert on mock presence rather than real behavior), incomplete mock objects, test-only methods added to production classes https://github.com/obra/superpowers/blob/main/skills/test-driven-development/testing-anti-patterns.md
• Trail of Bits / differential-review — contributed: anti-rationalization table ("small PR" / "just a refactor" rationalizations), git history check for security-sensitive removed code (commits tagged fix/CVE/auth/security as red flags) https://github.com/trailofbits/skills/tree/main/plugins/differential-review
• obra/superpowers — requesting-code-review / code-reviewer template — contributed: calibration note (lead with genuine praise before issues; authors trust critical feedback more when strengths are acknowledged accurately), backward compatibility and migration steps as a PR Quality check https://github.com/obra/superpowers/tree/main/skills/requesting-code-review
• agamm/claude-code-owasp — contributed: fail-closed error handling (deny/fail safely on network/parse failures, not silently proceed with stale data), supply chain check for new/updated dependencies (pinned version, auditable source, known CVEs) https://github.com/agamm/claude-code-owasp
• fcakyon/claude-code — pr-review-toolkit — contributed: recurring reviewer patterns check (use git commit log to find prior PR numbers on touched files, spot-check for repeated issues), fallback-masking-errors pattern (catch blocks that swallow exceptions and return stale/default data, hiding the underlying failure from callers) https://github.com/fcakyon/claude-code/tree/main/plugins/pr-review-toolkit
• ddclient/ddclient PR #618 review — contributed: stray trailing character in string-concatenated payload (invalid XML, silent 400), proxy bypass when protocol-level HTTP call omits caller-configured proxy, config key name mismatch between docs and implementation, unused declared config vars appearing in --help
• ddclient/ddclient PR #743 review — contributed: unused declared config vars, config key consistency (docs vs actual cfgvar names)