review-pr

name

review-pr

description

Review a php/php-src (or any GitHub) pull request and produce REVIEW.md. After writing the review, ask the user whether to post findings as inline PR comments via gh; never auto-posts. Use when the user says "review PR 12345", "/review-pr 12345", "review this pr", "code review for php-src PR N", "look at this pull request", or otherwise wants a structured review of a GitHub PR. Posts only on explicit user "yes"; Blockers + Majors by default, asks again before Minors, never posts Nits.

review-pr

Review a GitHub pull request against php-src coding standards. Produces REVIEW.md in pwd. Optionally posts findings as inline review comments via the gh CLI — only on explicit user confirmation.

Inputs

1. PR identifier (required) — a number (12345), #12345, or URL (https://github.com/php/php-src/pull/12345). Default repo is php/php-src.
2. gh CLI — must be authenticated (gh auth status).
3. $PHP_SRC_DIR — used to read surrounding context for the diff (file content beyond the hunk).

What it produces

• REVIEW.md in the task directory resolved per _shared/task-dir.md (default $PHP_SRC_DIR/.claude/php-core-dev/pr-<N>/REVIEW.md) — severity-graded findings, same format as review-local.
• Slug: pr-<N>. Written to .current.
• Optionally, on user "yes": inline review comments on the PR via gh pr review / gh api.

Workflow

1. Pre-checks

• gh auth status succeeds. Otherwise stop with Run: gh auth login.
• $PHP_SRC_DIR is set and looks like php-src.
• Argument resolves to a PR.

2. Fetch PR metadata + diff

gh pr view <PR-N> --repo php/php-src \
  --json number,title,body,state,baseRefName,headRefName,headRefOid,url,files,author,labels

gh pr diff <PR-N> --repo php/php-src

Record:

• Title, author, base branch, head branch (baseRefName / headRefName)
• Head SHA (headRefOid) — needed for posting inline comments
• File list with status (added / modified / removed / renamed)
• Total additions / deletions
• PR URL

If the PR is closed or merged, proceed but note it in the Scope section.

3. Backup existing REVIEW.md

test -f ./REVIEW.md && mv ./REVIEW.md ./REVIEW.md.bak

4. Read surrounding context

For each touched file, read the post-image from $PHP_SRC_DIR if the PR is against the same checked-out branch as the working tree. Otherwise, read via gh:

gh api /repos/php/php-src/contents/<path>?ref=<headRefOid> --jq '.content' | base64 -d

You only need to load files where hunks aren't self-contained (e.g. a hunk modifies one branch of an if and you need to see the other branch).

5. Analyse

Same rubric as review-local:

1. Correctness
2. Memory safety (emalloc/efree, zval lifecycle, refcounting, zend_string ownership)
3. API contracts (PHPAPI, header parity, BC)
4. PHP semantics (type juggling, error discipline, exception state, GC)
5. Thread-safety (ZTS build, ZEND_TLS, globals)
6. .phpt coverage (behavioural change without test is a Blocker unless provably untestable)
7. Coding standards (_shared/php-coding-standards.md)
8. Error message phrasing (canonical patterns)

Be conservative on severity. Anchor every finding to path:line in the post-image of the diff — these become inline review comments.

6. Write REVIEW.md

Follow _shared/review-md-format.md. type: review-pr, target: <PR URL>. Record headRefOid in the body so the user (or a future invocation) can reference the exact reviewed commit.

7. Offer to post

After writing, ask the user explicitly (one question, three options):

REVIEW.md written. Findings:
- <B> blocker, <M> major, <m> minor, <n> nit

Post to PR #<N>?
  1) Yes — Blockers + Majors only (recommended)
  2) Yes — Blockers + Majors + Minors
  3) No

If the user says "1" or equivalent: post Blockers + Majors as inline review comments. If "2": ask again specifically about Minors — Confirm: include <m> Minor finding(s)? (yes/no). Then post on yes. If "3" or anything ambiguous: don't post. Stop.

Never include Nits in a posted review. They live in REVIEW.md for the user's eye only.

8. Post (only on explicit "yes")

Build a JSON payload for gh api. Each inline comment maps to one finding with a path:line anchor. Use:

gh api -X POST /repos/php/php-src/pulls/<N>/reviews \
  -F commit_id=<headRefOid> \
  -F event=COMMENT \
  -f body="<summary header — one paragraph + counts>" \
  --jq '.id' > /tmp/review-id.txt

REVIEW_ID=$(cat /tmp/review-id.txt)

# Then per-finding:
gh api -X POST /repos/php/php-src/pulls/<N>/comments \
  -F commit_id=<headRefOid> \
  -F path=<path> \
  -F line=<line> \
  -F side=RIGHT \
  -F in_reply_to=$REVIEW_ID \
  -f body="<finding body>"

Use a single review (/reviews) that bundles the inline comments under it, rather than posting each finding as a top-level comment — keeps the PR conversation clean. The review's top-level body summarises severity counts; the inline comments carry the per-finding detail.

Default event: COMMENT (not REQUEST_CHANGES or APPROVE). The user can manually change the review state on github.com if they want to. Don't request changes on the author's behalf without explicit instruction.

If gh returns an error (rate limit, permissions, locked PR), report it and stop — the local REVIEW.md is still valid.

9. Report

Reviewed PR #<N>: <title>
Author: <author>   Branch: <head> → <base>
Findings: <B> blocker, <M> major, <m> minor, <n> nit
Posted: <none | B+M | B+M+m>
Local: ./REVIEW.md

Boundaries

• Never post without an explicit "yes". A user dropping a PR number is not consent to post.
• Never post Nits.
• Never use event=REQUEST_CHANGES or event=APPROVE unless the user explicitly asks.
• Read-only against $PHP_SRC_DIR. No checkouts, no branch switches.
• No edits in the PR's branch. Only comments.

Edge cases

Situation	Handling
PR is from a fork	gh pr diff works the same; posting still goes via php/php-src API path. commit_id is the fork's commit SHA, which gh api accepts.
PR has multiple commits	Post against the HEAD commit (headRefOid). Inline comments on stale commits are fine — GitHub shows them on the merged view.
User typed #12345 instead of 12345	Strip the #.
PR is closed / merged	Allow the review; note in Scope. Posting still works on closed PRs.
User says "yes" but with ambiguity ("post the blockers I guess")	Treat as 1 (Blockers + Majors). If the user said "just the blockers", post Blockers only. Always echo back the plan before posting.
gh api rate-limited	Stop posting; report the rate-limit window; the local REVIEW.md is the deliverable.
Author is the user themselves	Still review. Many php-src contributors review their own PRs before requesting feedback on internals@. Don't refuse.
PR touches the same diff a prior REVIEW.md covered	Note the prior review in the new one's Scope; backup is .bak.
Headerless diff or weird --unified=0 content	Re-fetch with gh pr diff --patch.

Quality checklist

• REVIEW.md frontmatter has all required fields with integer counts
• headRefOid recorded in REVIEW.md body
• Every finding anchored to path:line in the post-image
• User was asked explicitly before any post happened
• No event=REQUEST_CHANGES / event=APPROVE without user instruction
• No Nits in any posted review
• If posting failed, the local REVIEW.md is still complete

Out of scope

• Approving or merging the PR
• Posting comments on issues (vs. PRs)
• Modifying the PR's branch
• Reviewing the user's local diff — that's /review-local