theam/claude-dev-kit/pr-review

PR Review

Produce a high-signal review: findings a reviewer would act on, classified and ordered, ending in a clear verdict. The kit instructions (instructions/secure-coding.md, instructions/testing-standards.md) define what counts as blocking.

Scope the diff

• Reviewing an existing PR: fetch the diff and description from the configured host (prHost in .claude/dev-kit.json). github: gh pr diff <pr> + gh pr view <pr>. bitbucket: GET /2.0/repositories/{ws}/{repo}/pullrequests/{id}/diff and /pullrequests/{id} (REST, token from env). gitlab: glab mr diff <id> + glab mr view <id>.
• Reviewing the working tree (self-review before PR): git diff against the base branch, including staged changes.
• Read the linked ticket's acceptance criteria — a diff can be flawless and still not do what the story asked.
• Write the PR intent — one line on what this PR is for and what it deliberately leaves alone. It's the ruler for scope: a real defect inside the intent blocks; a valid concern outside it is a note or a follow-up, not a reason to expand the PR.

Review dimensions (in priority order)

1. Acceptance criteria: does the change actually satisfy each criterion? List any criterion not covered.
2. Correctness: behavioral regressions, broken edge cases, wrong logic. Read the code, don't skim the diff.
3. Contract drift: routes, payloads, enums, schemas, validation, status codes — every side that depends on the contract updated together.
4. Security: apply the checklist in instructions/secure-coding.md (auth on new endpoints, secrets, input validation, data exposure). Any automatic-blocker present is a blocking finding.
5. Tests (adaptive — judge against the project's own setup, see instructions/testing-standards.md): when the project has tests, every behavioral change has one that would fail without it and touched files stay at the project's bar (default ≥ 95%, no regression — run coverage-check if evidence is missing); when it does e2e, user-facing changes have e2e coverage with edge cases. A project with no test/e2e setup is not a blocking finding — flag it as a recommendation. Test-quality violations from instructions/testing-standards.md (assertion-free tests, suppressions, deleted/renamed tests) are findings.
6. Performance regressions introduced here (blocking): algorithmic blowups over collections that grow with usage; N+1 queries or per-item network calls on a request path; unbounded result sets / memory / missing pagination; blocking work on a hot path; a new query filtering/joining on an unindexed column. Not this: micro-optimizations or "could be faster" with no mechanism.
7. Duplication introduced by this PR (blocking): new code reimplementing logic already in the repo, or copy-paste between the files this PR adds — fix by reusing/extracting once. Not this: two blocks that merely look alike and are about to diverge; pre-existing duplication is a follow-up at most.
8. Accessibility (conditional — evaluate only when the diff changes user-facing UI in a frontend stack: changed components/templates/JSX/HTML/CSS in a node/angular/react/vue-style project. Skip entirely for backend or non-UI diffs — no cost when it doesn't apply). Honors a11y in .claude/dev-kit.json: auto (default — run on user-facing frontend diffs) · required (blocking gate) · off (never run). Check the high-value, low-effort basics on the changed markup only: images have meaningful alt, form controls have associated labels, interactive elements have an accessible name, keyboard/focus works (no click-only handlers, visible focus), no obvious color-contrast failures, ARIA present where needed and not misused. If the repo already runs a11y tooling (axe-core, eslint-plugin-jsx-a11y, Lighthouse), use its output; never scaffold one. Target WCAG 2.2 level AA, and cite the specific Success Criterion in each finding (e.g. missing alt → WCAG 1.1.1 (A), low contrast → WCAG 1.4.3 (AA)) so it's verifiable, not vague. Recommendation by default; blocking only when `a11y: required`.
9. Maintainability: only issues that materially affect future changes — no style nitpicks a formatter or linter should catch.

Adversarial check (proportional — do not double every review)

For high-stakes diffs only (auth/authorization, money, personal data, migrations, concurrency, anything hard to roll back), before the verdict take one targeted skeptical pass: pick the 1–2 conclusions most likely to be wrong and the 1–2 "looks fine" spots most likely to hide a defect, and actively try to break them (an edge input, a failure/timeout path, a race, a partial write). Routine or low-risk diffs get the normal single pass — this is a focused second look where being wrong is expensive, not a mandatory re-review.

Output format

## Review: <PR/diff identifier>
 
### Blocking
- [file:line] <finding> — <why it blocks, one line>
 
### Non-blocking
- [file:line] <suggestion>
 
### Questions
- <anything ambiguous that needs the author's intent>
 
### Verdict
APPROVE | REQUEST CHANGES — <one-line rationale>
Acceptance criteria: <met / partially met (which ones missing)>

Rules

• Report first, publish only with consent. Findings are delivered in the conversation. Never submit a GitHub review verdict (approve / request changes) and never post comments on the PR without the user's explicit confirmation — show exactly what would be posted and wait. This applies doubly to PRs authored by other people.
• Every finding cites file and line. No finding without a concrete failure scenario or rule reference.
• Do not pad: if the diff is clean, say so and approve — a review's value is its signal ratio.
• Never approve with unresolved blocking findings, and never report a criterion as met without seeing the code that implements it.
• When invoked as the self-review step of the story workflow, hand the blocking findings to the fix-pr playbook (the pr-fixer subagent on Claude Code) and re-review after the fixes.