pr-review

name

pr-review

description

Claude-side pre-PR review gate. Resolves the branch's base ref (gh PR view or explicit --base), pins an authoritative diff packet, then launches the pr-review dynamic workflow (~/.claude/workflows/pr-review.js) which fans out up to 8 review specialists (code-reviewer, security-reviewer, adversarial-reviewer, pr-test-analyzer, comment-analyzer, silent-failure-hunter, type-design-analyzer, code-simplifier), fails closed on coverage, and verifies Critical/Important findings. Use before creating a pull request as a quality gate for thick changes — especially where the Codex CLI `$pr-review` skill is unavailable. Triggers: "pre-PR review", "PR 前レビュー", "レビュー gate を回して", "run the pr-review gate".

pr-review (Claude-side)

Goal

Run a comprehensive specialist review of the current branch's committed changes against its base, then render a single actionable report. This is the Claude Code counterpart of the Codex $pr-review skill for environments where Codex is unavailable; both share one gate policy (references/review-criteria.md) and one severity table (references/severity-rules.json). Design: docs/design/claude-pr-review.md (dotfiles repo), decision: ADR 0029.

Architecture note (read first)

gh does NOT work inside workflow subagents (sandbox-pinned). Everything that needs gh or dangerouslyDisableSandbox happens HERE in the main session (Preconditions + Collect). The workflow receives all of it as args and uses only git. Do not move base resolution into the workflow.

Preconditions

Run these in order. If any fails, abort with the indicated error; do not launch the workflow.

1. Clean worktree — Run git status --porcelain --untracked-files=normal. If the command fails, abort with its output. If output is non-empty, abort with:

   "Worktree has uncommitted changes: . The review covers committed branch diff only; uncommitted changes would be silently excluded. Commit or stash first, then retry."

   Sandbox caveat: in repos whose tracked names collide with sandbox baseline denies (e.g. a chezmoi source dir), sandboxed git status reports ghost char-special entries (crw-rw-rw- nobody nogroup 1, 3 under ls -la) as untracked. Verify with ls -la | grep '^c' and re-run the status check outside the sandbox before trusting a non-empty result.

2. Base ref resolution — Determine $BASE from one of three sources, in priority order:

   • (a) Explicit base from the user prompt (e.g. --base develop, "review against develop"): use it verbatim and skip gh. If it matches a full 40-character hexadecimal commit OID (the same shape the workflow's args validation requires), set $BASE_REF to it. Otherwise validate it as a branch name (git check-ref-format --branch "$BASE"; reject leading -/+, :, or other refspec separators), run git fetch --quiet origin "refs/heads/$BASE" (abort on failure), set $BASE_REF=FETCH_HEAD, and resolve $BASE_COMMIT immediately.
   • (b) --allow-no-pr in the prompt: skip gh. Run git fetch --quiet origin and git remote set-head origin --auto (abort if either fails), then git symbolic-ref --quiet --short refs/remotes/origin/HEAD (abort if it fails or is empty). Strip origin/ for $BASE, keep the full origin/<branch> as $BASE_REF, and add a **Degraded coverage**: no PR base, fell back to default branch line to the final report.
   • (c) Open PR: run gh pr view --json baseRefName,baseRefOid --jq '[.baseRefName,.baseRefOid] | @tsv'. gh typically needs dangerouslyDisableSandbox: true (macOS Seatbelt / hosts.yml restrictions). If it fails with a real credential error, abort with "Run gh auth login and retry." Only an explicit "no pull request found" result is the no-PR case; any other failure aborts loudly. On success: validate the branch name with the same rules as (a), git fetch --quiet origin "refs/heads/$BASE" (abort on failure), verify FETCH_HEAD^{commit} equals the returned baseRefOid exactly (abort with both OIDs if not), then set $BASE_REF=FETCH_HEAD.

   If none of (a)–(c) yields a base, abort with:

   "No PR found for the current branch and no explicit base provided. Create a draft PR first (so the review pins the PR's base ref), provide an explicit base, or pass --allow-no-pr to fall back to the default branch (residual scope-divergence risk acknowledged)."

3. Pin the base commit — BASE_COMMIT=$(git rev-parse --verify "$BASE_REF^{commit}") immediately after assigning $BASE_REF; record the exact output. Abort on failure.

Collect (main session)

1. HEAD_REF=$(git rev-parse HEAD) — record it.

2. Empty diff check + changed files: run git diff --name-only "$BASE_COMMIT...$HEAD_REF" and record the output as changedFiles (one path per array element — this main-session list is the authoritative specialist-routing input; the workflow refuses to re-derive it from an agent echo). If empty, run the Final guard below, then report No committed changes relative to <base>; nothing to review. and stop.

3. Diff packet: diff_packet=$(mktemp "${TMPDIR:-/tmp}/pr-review-diff.XXXXXX") (suffix-free template — suffixed templates collide on BSD mktemp), then git diff "$BASE_COMMIT...$HEAD_REF" > "$diff_packet". Record byte count (wc -c) and SHA-256 (sha256sum / shasum -a 256). Abort if any step fails. The packet is authoritative for all specialists.

4. Deterministic routing flags (grep floor — the workflow OR-composes these with the categorizer agent's semantic judgment, so an agent false negative can never narrow the specialist fan-out; grep false positives only widen it, which is the safe direction):

   # The diff itself fails loud (abort-on-failure, same as steps 2-3); only the
   # grep filters get `|| true`, whose exit 1 legitimately means "no matching
   # lines". A combined pipeline would let a git failure (bad ref, transient FS
   # error) silently yield an empty flags_src — false/false flags identical to
   # a genuine no-match, eroding the deterministic floor exactly when the
   # environment misbehaves.
   flags_raw=$(mktemp "${TMPDIR:-/tmp}/pr-review-flagsraw.XXXXXX") || exit 1
   flags_src=$(mktemp "${TMPDIR:-/tmp}/pr-review-flags.XXXXXX") || exit 1
   # .md/.mdx excluded: prose uses 'type'/'class'/'schema' constantly and would
   # over-trigger the keyword patterns below. The strict 'a/', 'b/', '/dev/null'
   # header forms keep removed SQL/Lua comment lines ('-- x' rendered as '--- x')
   # from being eaten by a bare ^--- exclusion.
   git diff "$BASE_COMMIT...$HEAD_REF" -- ':(exclude)*.md' ':(exclude)*.mdx' > "$flags_raw" \
     || { rm -f "$flags_raw" "$flags_src"; echo "ABORT: git diff for routing flags failed" >&2; exit 1; }
   LC_ALL=C grep -E '^[+-]' "$flags_raw" \
     | LC_ALL=C grep -vE '^(\+\+\+|---) (a/|b/|/dev/null)' > "$flags_src" || true
   
   commentChanges=false
   LC_ALL=C grep -qE '^[+-][[:space:]]*(#|//|/\*|\*|<!--|;|--|"{3}|'\''{3})' "$flags_src" && commentChanges=true
   
   typeChanges=false
   LC_ALL=C grep -qE '^[+-][[:space:]]*(export[[:space:]]+)?(pub(\([^)]*\))?[[:space:]]+)?((abstract|final|data|sealed)[[:space:]]+)?(class|interface|struct|enum|trait)[[:space:]]+[A-Za-z_]' "$flags_src" && typeChanges=true
   LC_ALL=C grep -qE '(^|[^[:alnum:]_])type[[:space:]]+[A-Za-z_][A-Za-z0-9_]*[[:space:]]*(=|struct([^[:alnum:]_]|$)|interface([^[:alnum:]_]|$))' "$flags_src" && typeChanges=true
   LC_ALL=C grep -q '@dataclass' "$flags_src" && typeChanges=true
   LC_ALL=C grep -qiE '(^|[^[:alnum:]_])(create|alter)[[:space:]]+(table|schema)([^[:alnum:]_]|$)' "$flags_src" && typeChanges=true
   
   # Self-cleanup: the temp files are only ever read inside this block, and
   # their mktemp paths are unrecoverable after the Bash call returns.
   rm -f "$flags_raw" "$flags_src"
   
   # The readout doubles as the block's exit status: without it, a benign
   # no-match on the trailing `grep -q ... && ...` chains exits 1, and the
   # variables are shell-local — unobservable after the Bash call returns.
   echo "commentChanges=$commentChanges typeChanges=$typeChanges"
   

   Run the block verbatim (LC_ALL=C and POSIX classes keep BSD/GNU grep deterministic) and record both booleans from the echoed line. Bare type/schema keywords are deliberately NOT matched — JS/JSON diffs contain type: 'string' everywhere and the flag would be always-true; definition-anchored false negatives are rescued by the categorizer agent's OR inside the workflow.

5. Shared references (deploy-skew defense — file existence alone is not enough):

   • Read ~/.claude/skills/pr-review/references/review-criteria.md and verify it contains the sentinel PR_REVIEW_CRITERIA_SHARED_V1. Keep the full contents as criteria.
   • Read ~/.claude/skills/pr-review/references/severity-rules.json, parse it as JSON, and verify sentinel == "PR_REVIEW_SEVERITY_RULES_V1" and version == 1. Keep the parsed object as severityRules.
   • If either check fails, abort with:

     "Shared gate policy missing or stale at ~/.claude/skills/pr-review/references/. Run chezmoi apply -v to redeploy, then retry."

Launch the workflow

Invoke the Workflow tool with the script deployed at ~/.claude/workflows/pr-review.js (expand ~ to the absolute home path) and pass every value as real JSON (objects/arrays, not JSON-encoded strings):

Workflow({
  scriptPath: "<home>/.claude/workflows/pr-review.js",
  args: {
    base: "<$BASE>",
    baseCommit: "<$BASE_COMMIT>",
    headRef: "<$HEAD_REF>",
    packetPath: "<$diff_packet>",
    packetBytes: <byte count>,
    packetSha: "<sha256>",
    changedFiles: ["<path>", ...],
    commentChanges: <boolean from the Collect grep floor>,
    typeChanges: <boolean from the Collect grep floor>,
    criteria: "<contents of review-criteria.md>",
    severityRules: <parsed severity-rules.json object>
  }
})

The workflow validates args (including both sentinels) and fails closed on any coverage mismatch, so a thrown workflow error is a gate failure — report it verbatim and stop; never retry with weakened inputs or partial coverage.

The workflow runs in the background; wait for its completion notification before rendering. Do not start other work that mutates this repository while it runs.

Final guard (after the workflow returns)

1. Re-run git status --porcelain --untracked-files=normal. If non-empty, abort with:

   "Review subagents or concurrent tooling changed the worktree: . These changes were not part of the reviewed committed diff. Revert, commit, or stash them, then retry."

2. Re-run git rev-parse HEAD and compare with the recorded $HEAD_REF. If it differs, abort with:

   "HEAD changed during review: started at <old>, now <new>. The completed specialist results do not cover the current commit. Re-run the review."

3. Remove the diff packet temp file (the routing-flags block in Collect step 4 cleans up its own temp files).

Render the result

The workflow returns a structured object (argsContract, critical, important/importantOverflow/importantTotal, suggestions/suggestionsOverflow/suggestionsTotal, strengths, refuted, specialists, stage2Ran). Before rendering anything, verify argsContract is exactly PR_REVIEW_ARGS_V2; if it is missing or different, a stale deployed workflow handled the run and silently ignored the deterministic routing flags — abort with:

"Stale ~/.claude/workflows/pr-review.js deployed (argsContract mismatch). Run chezmoi apply -v, then re-run the review."

Otherwise render it as:

# PR Review: <branch> vs <base>

<**Degraded coverage** line if --allow-no-pr was used>

Specialists: <result.specialists, comma-separated> | scope `<result.scope>` | packet `<first 12 chars of packetSha>`

## Critical Issues (N found)
- [<specialist>] <why> [<file>:<line>]
  - Impact scope: <impact_scope>
  - Verified assumptions: <verified_assumptions>
  - Unverified assumptions: <unverified_assumptions; should be empty for Critical>
  - Verdict: <verdict>(<verdictReasoning>)
  - Suggested fix: <fix>

## Important Issues (shown X of <importantTotal>, cap 5)
- [<specialist>] <why> [<file>:<line>] — verdict: <verdict><, missing verification: ... if present>

<if importantOverflow is non-empty:>
### Beyond cap (verified but not prioritized)
- [<specialist>] <one-line summary of why> [<file>:<line>] — verdict: <verdict>

## Suggestions (shown X of <suggestionsTotal>, cap 3)
- [<specialist>] <why> [<file>:<line>]

## Refuted by verification (excluded from the fix queue)
- [<specialist>] <why> — <verdictReasoning>

## Strengths
- [<specialist>] <note>

## Recommended Action
1. Fix Critical issues first
2. Address Important issues
3. Consider Suggestions
4. Re-run this skill after fixes to verify prior Critical/Important findings
5. Stop when Critical 0 / Important 0; do not re-run for Suggestions only
6. If this is the third or later pass and Critical/Important findings still churn, escalate to human judgment

Omit empty sections (except render ## Critical Issues (0 found) explicitly — the absence of Criticals is the gate's headline). Findings with verdict: needs-verification and non-empty missingVerification stay in the fix queue as Important with their missing verification stated; never silently drop them, but do not render them as Critical until the missing proof exists. Other verdicts carrying missingVerification, or needs-verification without it, are invalid verifier outputs and must fail closed.

Re-review

On a re-run after fixes, prioritize confirming whether each prior Critical/Important finding was fixed, remains, or was intentionally rejected. Raise new findings only when they are clear merge blockers; do not extend the loop with new nits, style feedback, or optional refactors. If the result is Critical 0 / Important 0, stop the gate loop. If only Suggestions remain, do not re-run the gate just to chase them. On the third and later pass, if Critical/Important findings keep appearing or changing without stable blocker evidence, call out possible review churn and return the decision to a human maintainer. (This guidance also reaches the specialists via the bundled criteria.)