review-local

name

review-local

description

Review a local php-src diff (your current branch vs origin/master, or a base ref you provide) against PHP coding standards and produce REVIEW.md. Use when the user says "review my changes", "review this branch", "/review-local", "review the diff before I push", "check my fix", "what would internals reviewers say about this", or otherwise wants a structured code review of unpushed local php-src work. Read-only; never modifies source code or git state.

review-local

Review the user's current local php-src diff. Read-only. Produces REVIEW.md in pwd.

Inputs

1. $PHP_SRC_DIR — the php-src checkout.
2. Optional base ref — defaults to git merge-base HEAD origin/master. User can override (/review-local PHP-8.3).
3. The skill operates against the current branch in $PHP_SRC_DIR. It does NOT switch branches.

What it produces

• REVIEW.md in the task directory resolved per _shared/task-dir.md (default $PHP_SRC_DIR/.claude/php-core-dev/review-<short-base>/REVIEW.md) — severity-graded findings.
• If REVIEW.md exists from a prior run at the same path, it's backed up to REVIEW.md.bak before being overwritten.
• Slug: review-<short-sha> where <short-sha> is git rev-parse --short <base>. If the base is a branch (e.g. PHP-8.3 — any maintained branch from _shared/branch-detection.md), use review-<branch-slug> instead. Slug is written to .current.

Workflow

1. Pre-checks

test -n "$PHP_SRC_DIR" && test -d "$PHP_SRC_DIR/Zend" || { echo "Run /install"; exit 1; }
cd "$PHP_SRC_DIR"
git status --porcelain          # warn (don't block) if dirty

If the working tree is dirty, note it in REVIEW.md's Scope section — uncommitted changes are part of the diff to review.

2. Determine base ref

BASE=$(git merge-base HEAD origin/master 2>/dev/null || git merge-base HEAD master)

If the user passed an explicit base (PHP-8.3, a SHA, etc.), use that.

If no base can be determined (no master, no origin), stop with:

Cannot determine review base. Pass one explicitly: /review-local <base-ref>

3. Pull the diff

git -C "$PHP_SRC_DIR" diff "$BASE"...HEAD --stat
git -C "$PHP_SRC_DIR" diff "$BASE"...HEAD

Capture:

• Files changed (count, paths)
• Lines added / removed (from --stat)
• The full unified diff (for analysis)

If the diff is empty, stop with No changes between $BASE and HEAD — nothing to review.

4. Backup existing REVIEW.md

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

5. Analyse

Walk the diff hunk by hunk. For each, check against (in priority order):

1. Correctness — does the code do what its surroundings expect?
2. Memory safety — emalloc/efree pairing; zval init/destroy (ZVAL_*/zval_ptr_dtor); refcounting (Z_TRY_ADDREF_P, Z_DELREF_P); zend_string ownership.
3. API contracts — PHPAPI signatures, header consistency, BC for public symbols.
4. PHP semantics — type juggling, error-handling discipline (ValueError vs TypeError), exception state preservation, GC.
5. Thread-safety — when ZTS is built; globals via ZEND_TLS or *G(…).
6. .phpt coverage — behavioural changes without a corresponding test are a Blocker unless the change is provably untestable.
7. Coding standards — see _shared/php-coding-standards.md. K&R tabs, /* */ only, PHP_* vs ZEND_* macro choice, naming.
8. Error message phrasing — canonical patterns (must be of type X, must be between X and Y, …).

Severity rubric: see _shared/review-md-format.md. Be conservative — when in doubt, the lower severity. PHP internals reviewers read these by hand and a noisy review gets ignored.

6. Write REVIEW.md

Follow _shared/review-md-format.md exactly. Frontmatter, then severity-grouped Findings, then Suggested follow-ups and Open questions.

Anchor every finding with path:line from the post-image of the diff. Without anchors, findings aren't actionable.

7. Report

Review written to ./REVIEW.md
Files reviewed: <N>     (+<add> -<rem>)
Findings:       <B> blocker, <M> major, <m> minor, <n> nit
Backup:         ./REVIEW.md.bak (if prior review existed)

Boundaries

• Read-only. No edits in $PHP_SRC_DIR. No edits anywhere except ./REVIEW.md (and the backup).
• No git mutations. No branch switch, no checkout, no fetch (we trust whatever origin/master already is locally; if it's stale, that's on the user).
• No GitHub interaction. This skill operates entirely against the local repo. Posting to a PR is /review-pr's job.
• No "approve" / "request changes" verdict object. Reviewers approve PRs, not local branches. The severity counts speak for themselves.

Edge cases

Situation	Handling
Diff is huge (>1000 lines, >20 files)	Note in Scope; review proportionally — focus on the highest-risk hunks (memory, API, threading) and explicitly list which hunks were spot-checked vs. fully read.
Diff touches generated files (Zend/zend_vm_execute.h, configure)	Note in Scope that generated files are skipped; review the generator instead (zend_vm_gen.php, configure.ac).
Diff is entirely test changes	Still review — .phpt correctness matters. Lower the chance of Blocker findings; Major/Minor mostly.
origin/master is missing locally	Try origin/main, then bare master/main. If none works, ask for an explicit base.
Working tree dirty	Note it in Scope. The diff still includes only committed changes vs. base; uncommitted edits are separate (git diff without args). Ask the user whether to include uncommitted edits.
User asks to review only specific files	Filter the analysis to the named files; record the filter in Scope.
No changes between HEAD and base	Stop with Nothing to review.
File renamed or moved	Anchor to the post-image path; note the rename in the finding body.

Quality checklist

• REVIEW.md frontmatter has all required fields with integer counts
• Every finding is anchored to path:line
• Severity assignment follows the rubric (Blocker only for correctness/contract; speculative perf is Minor at best)
• Scope section names the base ref, files changed, line counts
• No drive-by suggestions to refactor unchanged code (use Suggested follow-ups for that)
• Prior REVIEW.md backed up to REVIEW.md.bak if it existed

Out of scope

• Posting comments to a GitHub PR — /review-pr
• Modifying code — never
• Running tests — bug-verify covers that for the bug flow