Skip to content

Commit 9f19a68

Browse files
tudorpopamsclaude
tudorpopams
and
claude
committed
feat: add review-pr skill with confidence scoring
Add /review-pr skill that reviews PRs across 8 categories: change file, V9 patterns, dependency layers, SSR safety, testing, API surface, accessibility, and security/quality. Produces a structured report with a 0-100 confidence score and APPROVE/REQUEST_CHANGES/COMMENT recommendation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e670e40 commit 9f19a68

3 files changed

Lines changed: 213 additions & 0 deletions

File tree

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

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
---
2+
name: review-pr
3+
description: Review a PR for correctness, pattern compliance, testing, accessibility, and safety. Produces a confidence score for merge readiness.
4+
argument-hint: <PR-number-or-branch>
5+
allowed-tools: Bash Read Grep Glob
6+
---
7+
8+
# Review a Pull Request
9+
10+
Review PR **$ARGUMENTS** and produce a confidence score for merge readiness.
11+
12+
## Phase 1: Gather PR Context
13+
14+
```bash
15+
# PR metadata
16+
gh pr view $ARGUMENTS --json title,body,author,labels,files,additions,deletions,baseRefName,headRefName,state,isDraft,number
17+
18+
# Changed files list
19+
gh pr diff $ARGUMENTS --name-only
20+
21+
# Full diff
22+
gh pr diff $ARGUMENTS
23+
24+
# CI status
25+
gh pr checks $ARGUMENTS
26+
```
27+
28+
## Phase 2: Classify PR Type
29+
30+
Determine the PR type from changed files and metadata:
31+
32+
| Type | Detection | Check scope |
33+
| ---------------- | -------------------------------------------------------------------- | ---------------------------------------------- |
34+
| **docs-only** | All files are `*.md`, `docs/**`, `**/stories/**`, `**/.storybook/**` | Change file only |
35+
| **test-only** | All files are `*.test.*`, `*.spec.*`, `**/testing/**` | Change file + test quality |
36+
| **bug-fix** | Branch starts with `fix/` or title contains "fix" | All checks, extra weight on tests |
37+
| **feature** | Branch starts with `feat/` or adds new exports | All checks, extra weight on API + patterns |
38+
| **refactor** | No new exports, restructures existing code | All checks, extra weight on no behavior change |
39+
| **config/infra** | Changes to CI, configs, scripts only | Change file + no regressions |
40+
41+
For **v8 packages** (`packages/react/`): skip V9 pattern checks — those are maintenance-only with different patterns.
42+
For **web-components** (`packages/web-components/`): skip React-specific checks.
43+
44+
## Phase 3: Run Checks
45+
46+
Run each check category. For each finding, assign a severity:
47+
48+
- **BLOCKER** — must fix before merge
49+
- **WARNING** — should address
50+
- **INFO** — consider
51+
52+
### A. Beachball Change File
53+
54+
Required if any published package source code changed (not just tests/stories/docs).
55+
56+
- Check `change/` directory in the diff for new `.json` files
57+
- Verify change type: `patch` for fixes, `minor` for features, never `major` without explicit approval
58+
- Not required for changes that only affect tests, stories, docs, or snapshots
59+
60+
**BLOCKER** if missing for published source changes.
61+
62+
### B. V9 Component Pattern Compliance
63+
64+
Only for files in `packages/react-components/react-*/library/src/`:
65+
66+
| Check | Look for | Severity |
67+
| ------------------- | --------------------------------------------------------------------------------------------------- | -------- |
68+
| No `React.FC` | `React.FC`, `: FC<`, `React.FunctionComponent` in added lines | BLOCKER |
69+
| No hardcoded styles | Hex colors `#[0-9a-fA-F]{3,8}`, hardcoded `px` values for spacing/radius/font in `.styles.ts` files | WARNING |
70+
| Griffel usage | Style files must use `makeStyles` from `@griffel/react`, not inline styles | WARNING |
71+
| mergeClasses order | User `className` must be the LAST argument in `mergeClasses()` | WARNING |
72+
| Slot system | New components must use `slot.always`/`slot.optional` and `assertSlots` | WARNING |
73+
74+
Reference: [docs/architecture/component-patterns.md](../../../docs/architecture/component-patterns.md)
75+
76+
### C. Dependency Layer Violations
77+
78+
For changes to `package.json` files or new imports in Tier 3 component packages:
79+
80+
- **BLOCKER** if a Tier 3 package (`react-button`, `react-menu`, etc.) adds a dependency on another Tier 3 package
81+
- Allowed Tier 2 deps: `react-utilities`, `react-theme`, `react-shared-contexts`, `react-tabster`, `react-positioning`, `react-portal`
82+
- Allowed Tier 1 deps: `@griffel/react`, `@fluentui/tokens`, `@fluentui/react-jsx-runtime`
83+
84+
Reference: [docs/architecture/layers.md](../../../docs/architecture/layers.md)
85+
86+
### D. SSR Safety
87+
88+
Grep added lines for unguarded browser API access:
89+
90+
| Pattern | Severity |
91+
| ------------------------------------------------------------- | -------- |
92+
| `window.` without `canUseDOM` or `typeof window` guard nearby | BLOCKER |
93+
| `document.` without guard | BLOCKER |
94+
| `navigator.` without guard | BLOCKER |
95+
| `localStorage` / `sessionStorage` without guard | BLOCKER |
96+
| `instanceof HTMLElement` | WARNING |
97+
98+
Check 3 lines above each match for a guard (`canUseDOM`, `typeof window !== 'undefined'`).
99+
100+
### E. Testing
101+
102+
| Check | Severity |
103+
| --------------------------------------------------------------------------- | -------- |
104+
| Source files changed but no corresponding `.test.tsx` changes | WARNING |
105+
| New component missing `testing/isConformant.ts` | WARNING |
106+
| Snapshot files need updating (render/style changes without `.snap` updates) | INFO |
107+
108+
### F. API Surface
109+
110+
| Check | Severity |
111+
| ------------------------------------------------- | -------- |
112+
| Public API changed but `etc/*.api.md` not updated | WARNING |
113+
| Existing exports removed (breaking change) | BLOCKER |
114+
| New exports added (flag for human review) | INFO |
115+
116+
### G. Accessibility
117+
118+
| Check | Severity |
119+
| -------------------------------------------------- | -------- |
120+
| Existing `aria-*` attributes removed | BLOCKER |
121+
| `onClick` without `onKeyDown`/`onKeyUp` handler | WARNING |
122+
| Interactive elements missing `role` or `aria-*` | WARNING |
123+
| Images/icons without `aria-label` or `aria-hidden` | WARNING |
124+
125+
### H. Security and Quality
126+
127+
| Check | Severity |
128+
| --------------------------------------------- | -------- |
129+
| `eval()` or `new Function()` | BLOCKER |
130+
| `dangerouslySetInnerHTML` | WARNING |
131+
| `console.log` / `debugger` in production code | WARNING |
132+
| `// @ts-ignore` without explanation | WARNING |
133+
| `any` type in new code | INFO |
134+
135+
## Phase 4: Calculate Confidence Score
136+
137+
```
138+
Start at 100
139+
140+
For each BLOCKER: -25 points
141+
For each WARNING: -5 points
142+
For each INFO: -1 point
143+
144+
Bonuses:
145+
+5 if tests added/updated alongside source changes
146+
+3 if change file present and well-described
147+
+2 if PR description is thorough
148+
149+
Floor at 0, cap at 100.
150+
```
151+
152+
Score interpretation:
153+
154+
- **90–100**: High confidence — safe to merge
155+
- **70–89**: Moderate confidence — minor concerns
156+
- **50–69**: Low confidence — needs attention
157+
- **0–49**: Not safe to merge — blockers present
158+
159+
## Phase 5: Produce Output
160+
161+
Use this exact format:
162+
163+
```
164+
## PR Review: #<number> — <title>
165+
166+
**Author:** <author>
167+
**Type:** <detected PR type>
168+
**Packages affected:** <list>
169+
**CI Status:** <passing/failing/pending>
170+
171+
### Confidence Score: <score>/100
172+
173+
<one-sentence summary>
174+
175+
### Findings
176+
177+
#### Blockers (must fix before merge)
178+
- [ ] <finding with file:line reference>
179+
180+
#### Warnings (should address)
181+
- [ ] <finding with file:line reference>
182+
183+
#### Info (consider)
184+
- <finding>
185+
186+
### Category Breakdown
187+
188+
| Category | Status | Notes |
189+
|----------|--------|-------|
190+
| Change file | PASS/FAIL | ... |
191+
| V9 patterns | PASS/WARN | ... |
192+
| Dep layers | PASS/FAIL | ... |
193+
| SSR safety | PASS/WARN | ... |
194+
| Testing | PASS/WARN | ... |
195+
| API surface | PASS/WARN | ... |
196+
| Accessibility | PASS/WARN | ... |
197+
| Security/Quality | PASS/WARN | ... |
198+
199+
### Recommendation
200+
201+
APPROVE / REQUEST_CHANGES / COMMENT
202+
203+
<brief rationale>
204+
```
205+
206+
## Notes
207+
208+
- For large PRs (50+ files), prioritize: published source files > test files > config. Note reduced confidence due to review scope.
209+
- Draft PRs: still review but note WIP status.
210+
- Merge conflicts: flag as BLOCKER if detected.
211+
- The `### Confidence Score: NN/100` line must always appear on its own line for machine parsing.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
@../../../.agents/skills/review-pr/SKILL.md

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ state.root.className = mergeClasses(
9999
| `token-lookup` | `/token-lookup val` | Find the design token for a hardcoded CSS value |
100100
| `package-info` | `/package-info pkg` | Quick lookup: path, deps, owner, tests, structure |
101101
| `visual-test` | `/visual-test Name` | Visually verify a component via Storybook + playwright-cli |
102+
| `review-pr` | `/review-pr #123` | Review a PR with confidence scoring and category checks |
102103

103104
## Package Layout
104105

0 commit comments

Comments
 (0)