|
| 1 | +--- |
| 2 | +name: review-pr |
| 3 | +description: Review a pull request for functional concerns. Use as a guide for reviewers or submitters preparing a PR. |
| 4 | +--- |
| 5 | + |
| 6 | +Review the specified pull request for **functional concerns only**. Ignore stylistic nits that don't impact correctness, performance, or maintainability. |
| 7 | + |
| 8 | +The PR number is provided as an argument. If no argument is given, check for an open PR on the current branch. |
| 9 | + |
| 10 | +## 1. Fetch PR context |
| 11 | + |
| 12 | +- Use `gh pr view <number> --json title,body,headRefName,baseRefName,files,additions,deletions` to get metadata |
| 13 | +- Use `gh pr diff <number>` to get the full diff |
| 14 | +- Read every changed file in the diff — don't skim |
| 15 | + |
| 16 | +## 2. Review focus areas |
| 17 | + |
| 18 | +Evaluate the diff against these categories. Only report findings that have real functional impact: |
| 19 | + |
| 20 | +| Category | What to look for | |
| 21 | +| --------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | |
| 22 | +| **Logic bugs** | Off-by-one errors, incorrect conditions, unreachable code, silent failures | |
| 23 | +| **Type safety** | Unsafe casts, type discrimination holes, `any` leaks, incorrect generics | |
| 24 | +| **Regex** | ReDoS potential, incorrect escaping, wrong flags, missing anchors | |
| 25 | +| **Performance** | Eager evaluation in hot paths, unnecessary allocations per-request, O(n^2) where O(n) is possible | |
| 26 | +| **State & concurrency** | Module-scoped mutable state, race conditions, missing cleanup | |
| 27 | +| **Code duplication** | Duplicated logic that will drift — only flag if >50 lines or contains branching logic | |
| 28 | +| **API contracts** | Breaking changes not flagged, silent behavior changes, incorrect error handling at boundaries | |
| 29 | +| **Security** | Injection vectors, credential handling, unsafe deserialization | |
| 30 | +| **Test coverage alignment** | Do the tests actually exercise the code paths that changed, or are they testing something adjacent? | |
| 31 | +| **Build artifact impact** | Does the change affect what ships in the npm package? New files in `src/` that aren't tree-shakeable, accidental inclusion of test files, etc. | |
| 32 | + |
| 33 | +## 3. Check contribution guidelines |
| 34 | + |
| 35 | +Read `CONTRIBUTING.md` at the repo root and verify the PR complies. Only check mechanical items: |
| 36 | + |
| 37 | +| Guideline | How to verify | |
| 38 | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | |
| 39 | +| **PR title format** | Must be `type(scope): description`. Type must be one of: build, ci, docs, feat, fix, perf, refactor, style, test. Scope must be a supported package name from CONTRIBUTING.md. | |
| 40 | +| **Tests included** | New functionality must have tests. Check the PR body's test plan for unchecked items. | |
| 41 | +| **Squash merge** | Preferred unless the PR explains why commit boundaries matter. | |
| 42 | +| **Related commits** | All commits should be related. Flag disjoint changes that should be separate PRs. | |
| 43 | +| **Linked issues** | The PR should reference related issues. Flag if "Closes #" is empty. | |
| 44 | +| **PR template** | Should include affected scope, test plan, and merge strategy recommendation. | |
| 45 | + |
| 46 | +## 4. Output format |
| 47 | + |
| 48 | +Write all findings in **second person**, actionable, ready to post as a review comment. |
| 49 | + |
| 50 | +- Code findings as a flat list grouped by severity |
| 51 | +- For each finding: |
| 52 | + - Name the category |
| 53 | + - Reference the specific file and code |
| 54 | + - Explain the functional impact |
| 55 | + - Suggest a fix if non-obvious |
| 56 | +- Mechanical guideline findings (title format, tests, linked issues, etc.) |
| 57 | + |
| 58 | +Skip categories with no findings. Do not pad the review with praise or filler. |
| 59 | + |
| 60 | +End with a verdict table: |
| 61 | + |
| 62 | +``` |
| 63 | +| Area | Verdict | |
| 64 | +|------|---------| |
| 65 | +| ... | ... | |
| 66 | +``` |
| 67 | + |
| 68 | +Keep verdicts to one phrase: "Clean", "Minor concern", "Should fix before merge", "Blocking". |
0 commit comments