| name | pr-review |
| description | Dual-axis change review — Standards (repo conventions, anti-patterns) and Spec (does the diff implement what the linked ADR/issue asked for) — evaluated in isolation so neither pass contaminates the other's verdict |
| allowed-tools | ["Read","Grep","Glob","Bash(git:*)","Bash(python3:*)"] |
| when_to_use | Use before merging any non-trivial change, or when asked to 'review this PR', 'check this diff', 'does this satisfy the ADR/issue', 'review my change'. Complements `rule-audit` (fixed D-01..D-37 rubric, whole-repo) and `security-audit` (security-only, pre-build): this skill is general-purpose, always runs two independent passes, and is the one to reach for on an ordinary feature/fix PR.
|
| argument-hint | <diff-ref-or-PR> [--spec <adr-or-issue-path>] |
| arguments | ["diff_ref","spec_ref"] |
| authorization_mode | {"review_standards":"AUTO","review_spec":"AUTO","post_review_comment":"CONSULT","escalation_triggers":[{"recommend_blocking_merge":"CONSULT"},{"finds_security_or_D17_18_19":"STOP"}]} |
PR Review Skill
Runs two reviews of the same diff in isolation — neither one sees the
other's notes before forming its own verdict — then reconciles them into
a single report. The isolation is the point: a reviewer who already knows
"this satisfies the ticket" tends to go easy on style; a reviewer who
already knows "the code is clean" tends to skip checking intent. Keeping
them apart is cheaper than a habit of catching it in your head.
Why two axes, not one checklist
- Standards answers: does this diff violate a convention or a known
anti-pattern (D-01..D-37), independent of what it's trying to do?
- Spec answers: does this diff actually do what the linked
ADR/issue asked for, independent of how clean the code is?
A diff can pass one and fail the other: a beautifully-typed function that
solves the wrong problem, or a correct fix that reintroduces D-24. Both
are real defects and neither axis alone catches both.
1. Resolve inputs (AUTO)
git diff <diff_ref>
If --spec was given, read that ADR/issue in full. If not, look for one
linked in the PR description or the branch name (ADR-\d+, issue #\d+);
if none exists, say so explicitly in the report — "no spec found" is a
finding, not a silent skip.
2. Pass A — Standards (AUTO, independent)
Check the diff against, in order:
AGENTS.md anti-patterns D-01..D-37 (the same rubric rule-audit
uses, scoped to just the changed lines/files).- The relevant
agentic/rules/*.md for any touched path (e.g. a
changed app/*.py file triggers 04a-python-serving.md).
- Repo-wide conventions: type hints, docstring style,
~= pinning,
lint/format (black, isort, flake8, mypy — run them if the
language toolchain is present).
Produce a list: file:line — violation — which rule/anti-pattern. Do
not yet read Pass B's notes.
3. Pass B — Spec (AUTO, independent)
Re-read the spec artifact (ADR/issue) and check, ignoring code style
entirely:
- Does the diff implement every acceptance criterion the spec states?
- Does it implement only those — flag scope creep (new abstractions,
unrelated refactors) the same way as a missing criterion.
- If the spec documented rejected alternatives, confirm the diff didn't
silently reintroduce one.
Produce a list: criterion — met / not met / partially met — evidence.
4. Reconcile (AUTO)
Only now combine both passes into one report:
## PR Review: <diff_ref>
### Standards
<Pass A findings, or "clean">
### Spec (vs. <spec_ref>)
<Pass B findings, or "no spec found">
### Verdict
<merge-ready | needs-changes | blocked>
A finding in either pass is enough to mark needs-changes. Only a
security/D-17/D-18/D-19 finding, or an explicit ask to enforce it,
escalates to recommending blocked — and that recommendation is
CONSULT, not a unilateral block.
5. Escalate (if triggered)
- Any secret, static credential, or unsigned-image-in-prod finding →
STOP, chain to
security-audit / secret-breach-response. Do not
include it as "just another Standards note."
- A recommendation to block the merge → CONSULT: state the reason
and wait for a human decision, never block unilaterally.
Success criteria
- Both passes ran and are reported before any combined verdict appears.
- Every Standards finding cites
file:line and the specific rule/D-NN.
- Every Spec finding cites the specific criterion from the spec artifact.
- No security-class finding was downgraded into a generic style note.
Related
- Skill:
rule-audit (fixed D-01..D-37 rubric, whole-repo, not diff-scoped)
- Skill:
security-audit (security-only, pre-build/pre-deploy)
- Skill:
diagnose-bug (when Standards review finds a bug, not just a style issue)
- Workflow:
/new-adr (when Spec review finds an undocumented decision)
