Skip to content

Commit 40b8a10

Browse files
authored
Add PR review skill skeleton (pytorch#22748)
### Summary Add a `/pr-review` Claude skill and route GitHub `@claude` review requests through it. This PR contains only the review infrastructure: - `.github/workflows/claude-code.yml`: allow the `Skill` tool and instruct Claude to use `/pr-review` for PR reviews (detailed review in a collapsible section, one-sentence summary outside). - `.claude/skills/pr-review/SKILL.md`: the review workflow (local CLI / local branch / GitHub Actions modes, sub-agent investigation, consolidation, fact-checking) and the output format. Adapted from the pytorch/pytorch `pr-review` skill with all PyTorch-specific content removed. - `.claude/skills/pr-review/review-checklist.md`: empty checklist; checks are added in stacked PRs. - `CLAUDE.md`: list `/pr-review`. The PyTorch checklist and BC guidelines are not imported: most of them (TensorIterator, `AT_DISPATCH`, `TORCH_CHECK`, OpInfo, autograd, Dynamo, etc.) do not apply to ExecuTorch and would produce incorrect findings. Stack: 1. pytorch#22748 Add PR review skill skeleton (this PR) 2. pytorch#22726 Flag wasteful GitHub Actions storage in PR reviews 3. pytorch#23315 Add generic checks to the PR review skill (for discussion) ### Test plan Docs/config only. Verified referenced files exist and the step numbering / cross-references in `SKILL.md` are consistent.
1 parent 273ecf9 commit 40b8a10

4 files changed

Lines changed: 254 additions & 0 deletions

File tree

‎.claude/skills/pr-review/SKILL.md‎

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
---
2+
name: pr-review
3+
description: Review ExecuTorch pull requests for issues CI cannot check. Use when reviewing PRs, when asked to review code changes, or when the user mentions "review PR", "code review", or "check this PR".
4+
---
5+
6+
# ExecuTorch PR Review Skill
7+
8+
Review ExecuTorch pull requests focusing on what CI cannot check, guided by the checklist in [review-checklist.md](review-checklist.md).
9+
10+
## Usage Modes
11+
12+
### No Argument
13+
14+
If the user invokes `/pr-review` with no arguments, **do not perform a review**. Instead, ask the user what they would like to review:
15+
16+
> What would you like me to review?
17+
> - A PR number or URL (e.g., `/pr-review 12345`)
18+
> - A local branch (e.g., `/pr-review branch`)
19+
20+
### Local CLI Mode
21+
22+
The user provides a PR number or URL:
23+
24+
```
25+
/pr-review 12345
26+
/pr-review https://github.com/pytorch/executorch/pull/12345
27+
```
28+
29+
For a detailed review with line-by-line specific comments:
30+
31+
```
32+
/pr-review 12345 detailed
33+
```
34+
35+
Use `gh` CLI to fetch PR data:
36+
37+
```bash
38+
# Get PR details
39+
gh pr view <PR_NUMBER> --json title,body,author,baseRefName,headRefName,files,additions,deletions,commits
40+
41+
# Get the diff
42+
gh pr diff <PR_NUMBER>
43+
44+
# Get PR comments
45+
gh pr view <PR_NUMBER> --json comments,reviews
46+
```
47+
48+
### Local Branch Mode
49+
50+
Review changes in the current branch that are not in `main`:
51+
52+
```
53+
/pr-review branch
54+
/pr-review branch detailed
55+
```
56+
57+
Use git commands to get branch changes:
58+
59+
```bash
60+
# Get current branch name
61+
git branch --show-current
62+
63+
# Get list of changed files compared to main
64+
git diff --name-only main...HEAD
65+
66+
# Get full diff compared to main
67+
git diff main...HEAD
68+
69+
# Get commit log for the branch
70+
git log main..HEAD --oneline
71+
72+
# Get diff stats (files changed, insertions, deletions)
73+
git diff --stat main...HEAD
74+
```
75+
76+
For local branch reviews:
77+
- The "Summary" should describe what the branch changes accomplish based on commit messages and diff
78+
- Use the current branch name in the review header instead of a PR number
79+
- All other review criteria apply the same as PR reviews
80+
81+
### GitHub Actions Mode
82+
83+
When invoked via `@claude /pr-review` on a GitHub PR, the action pre-fetches PR
84+
metadata and injects it into the prompt. Detect this mode by the presence of
85+
`<formatted_context>`, `<pr_or_issue_body>`, and `<comments>` tags in the prompt.
86+
87+
The prompt already contains:
88+
- PR metadata (title, author, branch names, additions/deletions, file count)
89+
- PR body/description
90+
- All comments and review comments (with file/line references)
91+
- List of changed files with paths and change types
92+
93+
Use git commands to get the diff and commit history. The base branch name is in the
94+
prompt context (look for `PR Branch: <head> -> <base>` or the `baseBranch` field).
95+
96+
```bash
97+
# Get the full diff against the base branch
98+
git diff origin/<baseBranch>...HEAD
99+
100+
# Get diff stats
101+
git diff --stat origin/<baseBranch>...HEAD
102+
103+
# Get commit history for this PR
104+
git log origin/<baseBranch>..HEAD --oneline
105+
106+
# If the base branch ref is not available, fetch it first
107+
git fetch origin <baseBranch> --depth=1
108+
```
109+
110+
Do NOT use `gh` CLI commands in this mode -- only git commands are available.
111+
All PR metadata, comments, and reviews are already in the prompt context;
112+
only the diff and commit log need to be fetched via git.
113+
114+
## Review Philosophy
115+
116+
A single line of code can have deep cross-cutting implications. **Treat every line as potentially load-bearing.**
117+
118+
1. **Only report problems** — The review output must contain only issues, concerns, and actionable suggestions. Do NOT mention things that are done correctly, do NOT praise good decisions, do NOT explain why something is fine. If a section has no problems, omit it entirely. The reader's time is precious — every sentence must point to something that needs fixing or further discussion.
119+
2. **Investigate, don't guess** — When uncertain whether a checklist item applies, spawn a sub-agent to read the relevant code. A reviewer who guesses wrong provides negative value.
120+
3. **Review the design, not just the implementation** — A PR can have perfectly correct implementation of a bad design. Question side-channel communication, on/off private flags, and demand concrete interface documentation for new contracts between components.
121+
4. **Focus on what CI cannot check** — Don't comment on formatting, linting, type errors, or CI failures. Focus on design quality, interface correctness, thread safety, BC implications, test adequacy, and pattern adherence.
122+
5. **Be specific and actionable** — Reference file paths and line numbers. Name the function/class/file the author should use.
123+
6. **Match the immediate context** — Read how similar features are already implemented in the same file. Pattern mismatches within a file are always wrong.
124+
7. **Assume competence** — The author knows ExecuTorch; explain only non-obvious context.
125+
8. **No repetition** — Each observation appears in exactly one section of the review output.
126+
127+
### Using sub-agents
128+
129+
The review checklist is large. You cannot hold the full context of every infrastructure system in your head. **Spawn sub-agents** to investigate whether checklist items apply: read surrounding code, infrastructure the PR should be using, or tests that should exist. Spawn them in parallel for independent areas. A typical medium PR should spawn 3-8 sub-agents.
130+
131+
Sub-agents overlap on purpose. Expect the same defect back from two or three of them in different words; that signals importance, not multiplicity. Reconcile in Step 3, before spending verification agents on it.
132+
133+
## Review Workflow
134+
135+
### Step 1: Understand Context
136+
137+
Before reviewing, build understanding of what the PR touches and why:
138+
1. Identify the purpose of the change from title/description/issue
139+
2. Group changes by type (new code, tests, config, docs)
140+
3. Note the scope of changes (files affected, lines changed)
141+
4. Spawn sub-agents to read the unchanged code surrounding each significantly changed file to understand existing patterns and infrastructure
142+
143+
### Step 2: Deep Review
144+
145+
Go through **every changed line** in the diff and evaluate it against the review checklist in [review-checklist.md](review-checklist.md).
146+
147+
### Step 3: Consolidate Findings
148+
149+
Deduplicate **before** you draft, and before Step 4 as it would slow it down significantly.
150+
151+
Flatten every candidate — yours and every sub-agent's — into a list of `(file:line, one-line claim)` pairs, then collapse it:
152+
153+
- **Same root cause → one finding.** Two sub-agents describing one defect from different angles is the expected outcome of fanning out over adjacent areas, not corroboration of two defects.
154+
- **Same fix → one finding.** If a single edit resolves several bullets, that is one finding with several consequences.
155+
- **Same `file:line` twice → merge**, unless you can name two independent defects.
156+
157+
Only then assign each survivor to exactly one section (see "One finding, one section" under Output Format) and write it up. Every finding must be traceable to a specific line in the diff.
158+
159+
### Step 4: Fact-Check
160+
161+
Fact-check the **consolidated** list — one sub-agent per surviving finding, never one per raw candidate.
162+
163+
Spawn the agents in parallel. Each independently verifies the claim by re-reading the relevant code and surrounding context, and returns **valid**, **invalid**, or **needs rewording**. Drop invalid issues, reword the rest. If unsure, leave the issue with a comment for the author that this is low confidence.
164+
165+
## Output Format
166+
167+
Structure your review as follows. **Omit sections where you have no problems to report** — most reviews should only have a few sections. Do not write "No concerns", "Looks good", or any affirmative commentary. Every sentence in the review must identify a problem or request a change.
168+
169+
The Summary section is the one exception: it should briefly state what the PR does (1 sentence) and then state the problems found, or explicitly say no issues were found.
170+
171+
```markdown
172+
## PR Review: #<number>
173+
<!-- Or for local branch reviews: -->
174+
## Branch Review: <branch-name> (vs main)
175+
176+
### Summary
177+
What the PR does (1 sentence), then the overall verdict.
178+
179+
### Code Quality
180+
[Problems only]
181+
182+
### Infrastructure
183+
[Problems only — flag checklist items that are violated]
184+
185+
### Testing
186+
[Problems only — missing tests, wrong patterns, inadequate coverage]
187+
188+
### API Design
189+
[Problems only]
190+
191+
### Security
192+
[Problems only]
193+
194+
### Thread Safety
195+
[Problems only]
196+
197+
### Backward Compatibility
198+
[Problems only]
199+
200+
### Performance
201+
[Problems only]
202+
203+
### Recommendation
204+
**Approve** / **Request Changes** / **Needs Discussion**
205+
206+
[Brief justification — focus on what blocks approval, if anything]
207+
```
208+
209+
**One finding, one section.** The categories overlap by construction — a missing test for a bad public API is legitimately API Design, Testing, and Code Quality. Assign each finding to exactly one section, first match wins:
210+
211+
1. **Security** — has a security consequence
212+
2. **Thread Safety** — has a concurrency consequence
213+
3. **Backward Compatibility** — breaks or risks breaking existing callers
214+
4. **API Design** — the defect is in a public or frozen interface (names, signatures, documented semantics)
215+
5. **Infrastructure** — a missing or incorrect hook into an existing ExecuTorch or CI subsystem
216+
6. **Testing** — the defect is *only* absent or inadequate coverage
217+
7. **Performance**
218+
8. **Code Quality** — everything else
219+
220+
State the finding's full consequence once, in its assigned section. When it genuinely spans categories, say so inline in that one bullet ("this is also a BC break for out-of-tree backends") rather than adding a second bullet under the other section.
221+
222+
This precedence governs the eight finding sections only. Summary and Recommendation are not finding buckets — see their rules in the template above. Neither is Specific Comments — see its rules below.
223+
224+
Do not invent sections outside this template. File a finding by its *consequence*, not its file type: a docstring that misstates operator semantics is API Design, not documentation. Where [review-checklist.md](review-checklist.md) groups or names a topic differently, that organizes the checklist, not the output; this precedence wins.
225+
226+
### Specific Comments (Detailed Review Only)
227+
228+
**Only include this section if the user requests a "detailed" or "in depth" review.**
229+
230+
**Do not repeat observations already made in other sections.** This section is for points too localized to carry their own categorized finding — single-line naming, wording, or stale-comment notes. Anything with a behavioral or interface consequence belongs in one of the eight sections above.
231+
232+
When requested, add file-specific feedback with line references:
233+
234+
```markdown
235+
### Specific Comments
236+
- `exir/example/module.py:78` - reuses the name of the enclosing loop variable
237+
- `runtime/example/example.cpp:203` - the error message says "tensor", but the parameter is a list
238+
- `backends/example/partitioner.py:512` - stale comment: describes the pre-refactor control flow
239+
```
240+
241+
## Files to Reference
242+
243+
When reviewing, consult these project files for context — read them rather than relying on memory, as they change frequently:
244+
- `CLAUDE.md` - Available skills and repository conventions
245+
- `CONTRIBUTING.md` - PR requirements and review process
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# PR Review Checklist
2+
3+
This checklist covers areas that CI cannot check. Skip items related to linting, formatting, type checking, and import ordering.

‎.github/workflows/claude-code.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ jobs:
1010
claude-code:
1111
uses: pytorch/test-infra/.github/workflows/_claude-code.yml@main
1212
with:
13+
additional_claude_args: --allowedTools Skill
14+
append_system_prompt: |
15+
When asked to review a PR, always use the /pr-review skill first.
16+
Put the detailed review inside a GitHub collapsible section and a
17+
one-sentence summary outside it.
1318
setup_script: |
1419
pip install lintrunner==0.12.7 lintrunner-adapters==0.14.1
1520
pip install -r requirements-lintrunner.txt

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
- `/profile` - Profile execution
88
- `/cortex-m` - Build, test, or develop the Cortex-M backend
99
- `/binary-size` - Analyze and reduce binary size
10+
- `/pr-review` - Review a pull request
1011
- `/qualcomm` - Build, test, or develop the QNN (Qualcomm AI Engine Direct) backend
1112
- `/executorch-kb` - Search tribal knowledge base (known issues, quant recipes, debugging guides)
1213

0 commit comments

Comments
 (0)