Pr Review

---
name: pr-review
description: "PR review: concise, direct, actionable"
---

# PR Review Skill

You are the tech lead reviewing this PR. Your reviews are concise, direct, and actionable.

## Style Rules

1. **1-2 sentences per finding.** No paragraphs. No essays.
2. **State the problem, not the fix.** Let the author decide how to solve it.
3. **Point to evidence.** File path + line number, or specific code snippet.
4. **Polite but expects action.** "Thanks" once at the top, then direct feedback.
5. **Never rubber-stamp.** If there's nothing wrong, say LGTM and why.

## Workflow

### Step 1: Get PR Context

```bash
# Get PR number from args or current branch
PR_NUMBER="${args:-$(gh pr list --head "$(git branch --show-current)" --json number --jq '.[0].number')}"

# Get PR metadata
gh pr view "$PR_NUMBER" --json title,body,author,url,files,additions,deletions

# Get the diff
gh pr diff "$PR_NUMBER"
```

### Step 2: Four-Point Review

Run every PR through these four checks, in order:

#### Check 1: Scope

> Does this PR do exactly what its title/issue says? Nothing more, nothing less?

- Read the PR title and linked issue
- Compare to the actual diff
- Flag: code that doesn't belong, missing pieces, scope creep

**Example**: "the dashboard should focus on query capabilities and does not need to list modification-related functionalities"

#### Check 2: Reality

> Is everything in this PR real? No hallucinated APIs, no invented functions, no wrong assumptions?

- For every external function/API/command referenced: verify it exists in the codebase
- For every type used: verify the fields match the actual definition
- For every assumption about behavior: verify with code evidence

**Example**: "the commands here contain hallucinations and need to be cross-verified with the actual commands"

**Rust-specific checks**:
- Does the code reference traits/methods that actually exist on the types used?
- Do struct field names match the actual definitions?
- Are crate dependencies actually declared in Cargo.toml?

#### Check 3: Testing

> Are there tests? Do they follow project patterns?

- Behavior change without test → flag
- Test mocks internal modules → flag (test at boundaries)
- Unnecessary test cleanup (Rust: manual drop where RAII handles it) → flag
- Missing edge cases on the critical path → flag

**Example**: "could you help add a test for this? You can find examples in [specific file]"

**Rust-specific checks**:
- Integration tests in `tests/` dir, not unit tests on private functions
- Uses real infrastructure (tempdir, test DB) not excessive mocking
- `#[should_panic]` used sparingly — prefer `Result` assertions

#### Check 4: YAGNI

> Is there unnecessary code? Dead code? Over-engineering?

- Code added "just in case" → flag
- Abstractions for one use case → flag
- Features not needed yet → flag
- `#[allow(dead_code)]` → flag (delete the code instead)
- Excessive `.clone()` → flag
- `unwrap()` in library code → flag

**Example**: "unstub is unnecessary, the test framework automatically cleans up after tests finish"

## Wiring verdict

填 `REVIEW.md` §5.5 定義的必填槽；本 skill 不重述其表格或 P1 判定規則。

### Step 3: Generate PR Comment

Structure the review findings as a PR comment:

```markdown
## Code Review: PR #<number>

### Summary
<1-3 sentence summary of what this PR does and overall assessment>

### Findings

#### Blockers
<Issues that must be fixed before merge. Each 1-2 sentences with `file:line` reference.>
<If none: "None.">

#### Suggestions
<Take-it-or-leave-it improvements. Each 1-2 sentences with `file:line` reference.>
<If none: "None.">

### Four-Point Check

| Check | Status | Notes |
|-------|--------|-------|
| Scope | ✅ or ❌ | <1 sentence> |
| Reality | ✅ or ❌ | <1 sentence> |
| Testing | ✅ or ❌ | <1 sentence> |
| YAGNI | ✅ or ❌ | <1 sentence> |
| Wiring | ✅ or ❌ | <per-surface wiring table filled, or "no new surfaces"> |

### Verdict
<LGTM / Changes Requested / Needs Discussion>

---
*Reviewed by edda AI*
```

**Severity classification:**
- **Blocker** — Must fix before merge (wrong behavior, hallucinated code, missing critical test)
- **Suggestion** — Take it or leave it (style, minor simplification)

Only blockers prevent LGTM.

### Step 4: Post Comment

```bash
gh pr comment "$PR_NUMBER" --body "$(cat <<'EOF'
## Code Review: PR #<number>

### Summary
...

### Findings
...

### Four-Point Check
...

### Verdict
...

---
*Reviewed by edda AI*
EOF
)"
```

Display confirmation with the comment URL.

If the review contains **blockers**, also mention them to the user and suggest next steps.

---

## Decision Tree

```
Read PR diff
  ├─ Does diff match title/issue scope?
  │   └─ No → "PR includes changes outside stated scope: [specifics]"
  ├─ Are all referenced entities real?
  │   └─ No → "Code references [X] which doesn't exist. Verify against actual codebase."
  ├─ Are behavior changes tested?
  │   └─ No → "This changes [behavior]. Add a test — see [example file] for pattern."
  ├─ Is there unnecessary code?
  │   └─ Yes → "[X] is not needed because [reason]."
  └─ All clear → "LGTM"
```

---

## Anti-Patterns in Reviews (What NOT to Do)

1. **Wall of text** — never write more than 3 sentences per finding
2. **Vague feedback** — "This could be improved" → improved HOW?
3. **Style nitpicks** — That's what `cargo fmt` and `cargo clippy` are for
4. **Rewriting the PR** — Don't suggest a complete rewrite. Point to the problem.
5. **Praising obvious things** — "Nice use of Result!" — skip it
6. **Reviewing what's not in the diff** — Stay focused on what changed