pr-review

Files (4)

• SKILL.md
• DETECTION-PLAYBOOK.md · references/
• QUALITY-FILTER.md · references/
• REVIEW-VOICE.md · references/

SKILL.md (first lines)

# Open Source PR Review

The general-purpose entry point of the suite. Every other skill in this repo is a
specialised lens layered on top of this procedure.

**Read first:** [`DETECTION-PLAYBOOK.md`](references/DETECTION-PLAYBOOK.md) ·
[`QUALITY-FILTER.md`](references/QUALITY-FILTER.md) ·
[`REVIEW-VOICE.md`](references/REVIEW-VOICE.md)

> **Where the time goes.** Finding the flaw is the review. The playbook above is the
> general flaw-hunting procedure; this file is the domain-specific lens on top of it.
> Scoring and writing the comment are the last ten minutes, not the work.

**Severity floor: 8.0.** Comment budget: one per ~200 changed lines, max 5.

---

## Why this exists

Default AI review output lands at **6.0–7.5/10**. It is polite, plausible, and mostly
unactionable — praise, redundant lints, micro-refactors that generate notification noise
for people maintaining a project in their spare time. The goal here is not more feedback.
It is *less* feedback, each piece of which is worth reading.

### What each role gets out of it

| Role | Time this gives back |
| :--- | :--- |
| **Maintainer** | A pre-filtered review. Instead of reading 14 comments to find the 2 that matter, you read 2. Context (tests run, baseline established, conventions checked) is already in the summary. |
| **Contributor** | A self-check before you open the PR, so the first maintainer response is "merged" rather than a round-trip on something you could have caught. |
| **Project owner** | A written, auditable review standard new reviewers can be pointed at — so review quality does not depend on who happened to pick up the PR. |

---

## Procedure

```mermaid
flowchart TD
    A[Start] --> B[1. Isolate the true diff]
    B --> C[2. Build context: repo conventions]
    C --> D[3. Empirical verification: build + tests]
    D --> E[4. Read existing comments]
    E --> F[5. Line-by-line audit]
    F --> G[6. Score with the 10-point filter]
    G -->|"score below 8.0"| H[Discard or batch to an issue]
    G -->|"score 8.0 or above"| I[7. Frame in review voice]
    I --> J[8. Get explicit approval, then post]
```

### 1. Isolate the true diff

Never review local branch state or a rebase merge commit. Review the exact three-dot diff
against the upstream target.

```bash
gh pr checkout <PR_NUMBER>
# or, without gh:
git fetch upstream 'refs/pull/<PR_NUMBER>/head:pr-<PR_NUMBER>'

git diff upstream/main...pr-<PR_NUMBER>              # full diff
git diff --name-only upstream/main...pr-<PR_NUMBER>  # touched files
git diff --stat upstream/main...pr-<PR_NUMBER>       # size, sets the comment budget
```

`pr-<PR_NUMBER>` is the branch name created by the `git fetch` form above. `gh pr checkout`
names the branch after the PR's *head* branch instead, so after `gh pr checkout` use
`upstream/main...HEAD` in place of `upstream/main...pr-<PR_NUMBER>`.

Three dots, not two. Two dots includes everything that landed on `main` since the branch
point, and you will end up reviewing other people's code.

> **Rule:** never target an inline comment at a line that is not added or context-adjacent
> in this diff. The GitHub API rejects it, and the author sees a broken thread.

### 2. Build context before judging

Ten minutes here removes most false positives.

```bash
cat CONTRIBUTING.md AGENTS.md CLAUDE.md 2>/dev/null   # explicit house rules
ls .github/workflows/                                  # what CI actually enforces
cat pyproject.toml setup.cfg .eslintrc* 2>/dev/null    # configured lint/format rules
git log --oneline -20 -- <file being changed>          # how this file usually evolves
```

If a linter already enforces it, do not comment on it. If `CONTRIBUTING.md` explicitly
permits it, do not comment on it.

### 3. Empirical verification

Never assert that code breaks based on reading alone.

```bash
# whatever the repo actually uses — read the CI config, not your habits
uv run pytest -q          # or: make test / npm test / go test ./... / cargo test
```

- Establish the baseline: do tests pass on `main` *before* the change? A pre-existing
  failure attributed to the PR is the most damaging mistake a reviewer can make.
- If you propose a fix, apply it locally and confirm it works before suggesting it.
- If you cannot run the suite, say so: "couldn't run the Go tests locally, so this is a
  read-only pass."

### 4. Read what has already been said

```bash
gh api /repos/<OWNER>/<REPO>/pulls/<PR>/comments \
  --jq '.[] | "\(.user.login) | \(.path):\(.line) | \(.body[0:150])"'

gh api /repos/<OWNER>/<REPO>/issues/<PR>/comments \
  --jq '.[] | "\(.user.login): \(.body[0:150])"'

gh pr view <PR> --json reviews --jq '.reviews[] | "\(.author.login) \(.state)"'
```

Duplicating Copilot or a maintainer is worse than staying silent — it signals you did not
read the thread.

### 5. Line-by-line audit

From priyank766/OpenSource-SKILL, Apache-2.0.

More from priyank766/OpenSource-SKILL