Claude Skill

meta-reviewing-reviewing

Code review patterns, feedback principles. Use when reviewing PRs, implementations, or making approval/rejection decisions. Covers self-correction, progress tracking, feedback principles, severity levels.

LLM Mart · 0 points · 0 0 0

Virus-scanned Reviewed automatically before listing.

Full trust report

Download agents-inc-skills-dist_plugins_meta-reviewing-reviewing_skills_meta-reviewing-reviewing-3a51ef5.zip · 11 KB

agents-inc/skills 24 8 forks MIT Updated 22d ago

Part of agents-inc/skills — 130 skills

Install

skills CLI

npx skills add https://github.com/agents-inc/skills/tree/main/dist/plugins/meta-reviewing-reviewing/skills/meta-reviewing-reviewing

Claude Code claude plugin marketplace add https://llmmart.ai/marketplace.json && claude plugin install agents-inc-skills@llmmart

Git git clone https://github.com/agents-inc/skills.git

The skills CLI installs just this skill, for any of its supported agents. Claude Code installs the whole agents-inc/skills collection as a plugin from our marketplace. Git is the plain clone.

Skill manifest

Reviewing Patterns

Quick Guide: Read ALL files completely before commenting. Provide specific file:line references for every issue. Distinguish severity (Must Fix vs Should Fix vs Nice to Have). Explain WHY, not just WHAT. Suggest solutions following existing patterns. Acknowledge good work - positive reinforcement teaches what to repeat.

<critical_requirements>

CRITICAL: Before Any Review

(You MUST read ALL files mentioned in the PR/spec completely before providing feedback)

(You MUST provide specific file:line references for every issue found)

(You MUST distinguish severity: Must Fix vs Should Fix vs Nice to Have)

(You MUST explain WHY something is an issue, not just WHAT is wrong)

(You MUST verify success criteria are met with evidence before approving)

(You MUST acknowledge what was done well - not just issues)

</critical_requirements>

Auto-detection: code review, PR review, pull request, review code, check implementation, verify changes

When to use:

Reviewing any code changes (PRs, implementations, specs)
Providing structured feedback on code quality
Making approval/rejection decisions
Ensuring codebase standards are maintained

When NOT to use:

When implementing code (use developer skills instead)
For automated linting/type-checking (use CI/CD tooling)
For deep security audits (use dedicated security review)
For high-level architecture decisions (use planning/PM workflows)

Key patterns covered:

Self-correction checkpoints for reviewers
Post-action reflection after reviews
Progress tracking for multi-file reviews
Feedback principles (specific, explain why, suggest solutions, severity, acknowledge good)
Decision framework for approval/rejection
Review-specific anti-patterns (scope creep, refactoring, not using utilities)

Detailed Resources:

examples/core.md - All examples: progress tracking, feedback patterns, anti-patterns

<decision_framework>

Decision Framework

Is this a blocking issue?
├─ YES → Does it affect security, functionality, or required criteria?
│   ├─ Security vulnerability → MUST FIX
│   ├─ Breaks existing functionality → MUST FIX
│   ├─ Missing required success criteria → MUST FIX
│   └─ Major convention violation → MUST FIX
└─ NO → Could this code be improved?
    ├─ YES → Is the churn worth the diff's purpose? Does the spec ask for it?
    │   ├─ NO to either → DON'T MENTION
    │   └─ YES → What is the impact?
    │       ├─ Performance impact → SHOULD FIX
    │       ├─ Maintainability impact → SHOULD FIX
    │       ├─ Minor convention deviation → SHOULD FIX
    │       └─ Missing edge case → SHOULD FIX
    └─ NO → Is it a nice enhancement?
        ├─ Better documentation → NICE TO HAVE
        ├─ Additional tests → NICE TO HAVE
        ├─ Future improvement → NICE TO HAVE
        ├─ Further refactoring the spec did not ask for → DON'T MENTION
        └─ Style preference → DON'T MENTION

Approval Decisions

APPROVE when:

All success criteria are met with evidence
Code follows existing conventions
No critical security or performance issues
Tests are adequate and passing
Changes are within scope

REQUEST CHANGES when:

Success criteria not fully met
Convention violations exist
Quality issues need addressing
Test coverage inadequate

MAJOR REVISIONS NEEDED when:

Critical security vulnerabilities
Breaks existing functionality
Major convention violations
Fundamental approach issues

When uncertain: Request changes with specific questions rather than blocking indefinitely.

</decision_framework>

<red_flags>

RED FLAGS

High Priority Issues:

Providing feedback without reading the full file
No file:line references in issue descriptions
Approving without verifying success criteria
Only negative feedback, no acknowledgment of good work
Reviewing code outside your domain expertise
Blocking PRs for personal style preferences

Medium Priority Issues:

Missing severity distinctions (all issues look equal)
No suggested solutions for identified issues
Vague feedback ("this needs improvement")
Not checking for existing patterns before flagging "new code"
Incomplete review (not all files examined)

Common Mistakes:

Assuming code behavior without reading implementation
Flagging valid patterns as "wrong" because unfamiliar
Missing obvious issues while focusing on minor ones
Not acknowledging improvement over previous versions
Providing contradictory feedback (fix X, but also don't change Y)

Gotchas & Edge Cases:

Some "duplication" is intentional for clarity - verify before flagging
Performance optimizations may not be needed for low-traffic code
"Convention violations" may be new patterns not yet documented
Test coverage percentages don't guarantee quality tests
"Out of scope" changes may be necessary dependencies

</red_flags>

<critical_reminders>

CRITICAL REMINDERS

(You MUST read ALL files mentioned in the PR/spec completely before providing feedback)

(You MUST provide specific file:line references for every issue found)

(You MUST distinguish severity: Must Fix vs Should Fix vs Nice to Have)

(You MUST explain WHY something is an issue, not just WHAT is wrong)

(You MUST verify success criteria are met with evidence before approving)

(You MUST acknowledge what was done well - not just issues)

Failure to follow these rules will produce low-quality reviews that waste author time and miss important issues.

</critical_reminders>

Files (skills)

examples

anti-patterns.md 3.7 KB

# Reviewing Patterns - Anti-Patterns

[Back to SKILL.md](../SKILL.md) | [core.md](core.md) | [feedback-patterns.md](feedback-patterns.md) | [reference.md](../reference.md)

> Common issues to flag in reviewed code, and reviewer behaviors to avoid.

---

## Code Anti-Patterns (What to Flag)

Watch for these common issues in reviewed code.

### Scope Creep

Code adds features not in the specification.

```typescript
// Spec requested: Email validation, Name/bio editing, Save functionality

// FLAGGED - Added unrequested features:
- Phone validation (not in spec)
- Avatar upload (not in spec)
- Password change (not in spec)
```

**Flag when:** Features not in original specification are implemented.

---

### Refactoring Existing Code

Working code was changed without being in scope.

```diff
- // Existing working code was changed
+ // "Improved" version that wasn't requested
```

**Flag when:** Changes beyond specified scope, "improvements" not requested.

---

### Not Using Existing Utilities

New code duplicates functionality that already exists.

```typescript
// FLAGGED - Reinvented the wheel
function validateEmail(email: string) {
  // Custom regex validation
}

// Should use existing utility
import { validateEmail } from "@/lib/validation";
```

**Flag when:** Code duplicates existing functionality instead of reusing.

---

### Modifying Out of Scope Files

Files changed that weren't mentioned in specification.

```typescript
// FLAGGED - Changed file not mentioned in spec
// auth.py was modified
// Spec said: "Do not modify authentication system"
```

**Flag when:** Files changed that weren't mentioned in specification.

---

### Missing Error Handling

API calls and async operations lack error handling.

```typescript
// FLAGGED - No error handling
const data = await apiClient.put("/users/123", formData);

// Should include error handling
try {
  const data = await apiClient.put("/users/123", formData);
  showSuccessMessage("Profile updated");
} catch (error) {
  showErrorMessage(error.message);
}
```

**Flag when:** API calls, async operations lack error handling.

---

## Reviewer Anti-Patterns (What to Avoid)

### Reviewing Without Reading Full Files

```markdown
# ANTI-PATTERN: Feedback based on partial reading

"The validation logic seems incomplete" <- Didn't read the whole file

# Later in file (line 145):

const { validateEmail, validatePhone } = useValidation(); <- Missed this
```

**Why it's wrong:** Incomplete context leads to incorrect feedback, wastes author time addressing non-issues.

**What to do instead:** Read ALL files completely before providing any feedback.

---

### Vague Feedback Without References

```markdown
# ANTI-PATTERN: No specific location

"This code needs improvement"
"There are some issues with the types"
"The error handling could be better"
```

**Why it's wrong:** Author doesn't know what to fix, feedback is not actionable.

**What to do instead:** Provide specific file:line references for every issue.

---

### All Issues Same Severity

```markdown
# ANTI-PATTERN: Everything treated as blocker

- Fix the typo in comment
- Add security validation <- Critical!
- Use more descriptive variable name
- Fix XSS vulnerability <- Critical!
```

**Why it's wrong:** Critical issues get lost among trivial ones, PR blocked by minor issues.

**What to do instead:** Clearly distinguish Must Fix vs Should Fix vs Nice to Have.

---

### Only Negative Feedback

```markdown
# ANTI-PATTERN: No acknowledgment of good work

- Fix line 23
- Fix line 45
- Fix line 67
- LGTM (after author fixes everything)
```

**Why it's wrong:** Demotivates author, misses teaching opportunity about what to repeat.

**What to do instead:** Acknowledge what was done well alongside issues.

core.md 5.7 KB

# Reviewing Patterns - Examples

> All examples for code review workflow: progress tracking, feedback patterns, and anti-patterns.

---

## Progress Tracking Example

```
Files Examined:
- [x] src/components/user-profile.tsx (complete)
- [x] src/hooks/use-user.ts (complete)
- [ ] src/api/users.ts (deferred to backend-reviewer)

Success Criteria:
- [x] User profile displays correctly
- [x] Edit form validates input
- [ ] Tests pass (need to verify)

Issues Found:
- 1x Must Fix (missing error handling)
- 2x Should Fix (performance optimizations)

Positive Patterns:
- Clean component structure
- Good TypeScript usage
```

---

## Retrieval Strategy

When reviewing unfamiliar code, use this systematic approach.

**Just-in-Time Loading:**

1. **Glob** - Find files by pattern (`**/*.ts`, `**/components/**`)
2. **Grep** - Search for function definitions, imports, error handling patterns
3. **Read** - Examine full file content before commenting

**Load patterns just-in-time** - Don't read everything upfront; load when relevant.

---

## Feedback Examples

### Be Specific

```markdown
Bad: "This code needs improvement"
Good: "ProfileEditModal.tsx line 45: This validation logic duplicates
validateEmail() from validation.ts. Use the existing utility instead."
```

**Why:** Vague feedback wastes time. Specific feedback can be acted on immediately.

---

### Explain Why

```markdown
Bad: "Don't use any types"
Good: "Line 23: Replace `any` with `UserProfile` type. This provides type
safety and catches errors at compile time. The type is already
defined in types/user.ts."
```

**Why:** Understanding impact helps authors learn and prevents repeat mistakes.

---

### Suggest Solutions

```markdown
Bad: "This is wrong"
Good: "Line 67: Instead of creating a new error handler, follow the pattern
in SettingsForm.tsx (lines 78-82) which handles this scenario."
```

**Why:** Solutions (especially referencing existing code) make fixes faster and maintain consistency.

---

### Acknowledge Good Work

```markdown
- "Excellent use of the existing validation pattern"
- "Good error handling following our conventions"
- "Tests are comprehensive and well-structured"
- "Clean implementation matching the pattern"
```

**Why:** Positive reinforcement teaches what to repeat. Reviews that only criticize demotivate and miss teaching opportunities.

---

## Code Anti-Patterns (What to Flag)

Watch for these common issues in reviewed code.

### Scope Creep

Code adds features not in the specification.

```typescript
// Spec requested: Email validation, Name/bio editing, Save functionality

// FLAGGED - Added unrequested features:
- Phone validation (not in spec)
- Avatar upload (not in spec)
- Password change (not in spec)
```

**Flag when:** Features not in original specification are implemented.

---

### Refactoring Existing Code

Working code was changed without being in scope.

```diff
- // Existing working code was changed
+ // "Improved" version that wasn't requested
```

**Flag when:** Changes beyond specified scope, "improvements" not requested.

---

### Not Using Existing Utilities

New code duplicates functionality that already exists.

```typescript
// FLAGGED - Reinvented the wheel
function validateEmail(email: string) {
  // Custom regex validation
}

// Should use existing utility from the codebase
```

**Flag when:** Code duplicates existing functionality instead of reusing.

---

### Modifying Out of Scope Files

Files changed that weren't mentioned in specification.

```typescript
// FLAGGED - Changed file not mentioned in spec
// auth.py was modified
// Spec said: "Do not modify authentication system"
```

**Flag when:** Files changed that weren't mentioned in specification.

---

### Missing Error Handling

API calls and async operations lack error handling.

```typescript
// FLAGGED - No error handling
const data = await apiClient.put("/users/123", formData);

// Should include error handling
try {
  const data = await apiClient.put("/users/123", formData);
  showSuccessMessage("Profile updated");
} catch (error) {
  showErrorMessage(error.message);
}
```

**Flag when:** API calls, async operations lack error handling.

---

## Reviewer Anti-Patterns (What to Avoid)

### Reviewing Without Reading Full Files

```markdown
# ANTI-PATTERN: Feedback based on partial reading

"The validation logic seems incomplete" <- Didn't read the whole file

# Later in file (line 145):

const { validateEmail, validatePhone } = useValidation(); <- Missed this
```

**Why it's wrong:** Incomplete context leads to incorrect feedback, wastes author time addressing non-issues.

**What to do instead:** Read ALL files completely before providing any feedback.

---

### Vague Feedback Without References

```markdown
# ANTI-PATTERN: No specific location

"This code needs improvement"
"There are some issues with the types"
"The error handling could be better"
```

**Why it's wrong:** Author doesn't know what to fix, feedback is not actionable.

**What to do instead:** Provide specific file:line references for every issue.

---

### All Issues Same Severity

```markdown
# ANTI-PATTERN: Everything treated as blocker

- Fix the typo in comment
- Add security validation <- Critical!
- Use more descriptive variable name
- Fix XSS vulnerability <- Critical!
```

**Why it's wrong:** Critical issues get lost among trivial ones, PR blocked by minor issues.

**What to do instead:** Clearly distinguish Must Fix vs Should Fix vs Nice to Have.

---

### Only Negative Feedback

```markdown
# ANTI-PATTERN: No acknowledgment of good work

- Fix line 23
- Fix line 45
- Fix line 67
- LGTM (after author fixes everything)
```

**Why it's wrong:** Demotivates author, misses teaching opportunity about what to repeat.

**What to do instead:** Acknowledge what was done well alongside issues.

feedback-patterns.md 1.5 KB

# Reviewing Patterns - Feedback Examples

[Back to SKILL.md](../SKILL.md) | [core.md](core.md) | [anti-patterns.md](anti-patterns.md) | [reference.md](../reference.md)

> Examples demonstrating effective feedback principles.

---

## Be Specific

```markdown
Bad: "This code needs improvement"
Good: "ProfileEditModal.tsx line 45: This validation logic duplicates
validateEmail() from validation.ts. Use the existing utility instead."
```

**Why:** Vague feedback wastes time. Specific feedback can be acted on immediately.

---

## Explain Why

```markdown
Bad: "Don't use any types"
Good: "Line 23: Replace `any` with `UserProfile` type. This provides type
safety and catches errors at compile time. The type is already
defined in types/user.ts."
```

**Why:** Understanding impact helps authors learn and prevents repeat mistakes.

---

## Suggest Solutions

```markdown
Bad: "This is wrong"
Good: "Line 67: Instead of creating a new error handler, follow the pattern
in SettingsForm.tsx (lines 78-82) which handles this scenario."
```

**Why:** Solutions (especially referencing existing code) make fixes faster and maintain consistency.

---

## Acknowledge Good Work

```markdown
- "Excellent use of the existing validation pattern"
- "Good error handling following our conventions"
- "Tests are comprehensive and well-structured"
- "Clean implementation matching the pattern"
```

**Why:** Positive reinforcement teaches what to repeat. Reviews that only criticize demotivate and miss teaching opportunities.

reference.md 5.7 KB

# Reviewing Patterns - Reference

[Back to SKILL.md](SKILL.md) | [examples/core.md](examples/core.md) | [examples/feedback-patterns.md](examples/feedback-patterns.md) | [examples/anti-patterns.md](examples/anti-patterns.md)

> Decision frameworks, red flags, and detailed guidance for code reviews.

---

<decision_framework>

## Decision Framework

```
Is this a blocking issue?
├─ YES → Does it affect security, functionality, or required criteria?
│   ├─ Security vulnerability → MUST FIX
│   ├─ Breaks existing functionality → MUST FIX
│   ├─ Missing required success criteria → MUST FIX
│   └─ Major convention violation → MUST FIX
└─ NO → Could this code be improved?
    ├─ YES → Is it worth the author's time?
    │   ├─ Performance impact → SHOULD FIX
    │   ├─ Maintainability impact → SHOULD FIX
    │   ├─ Minor convention deviation → SHOULD FIX
    │   └─ Missing edge case → SHOULD FIX
    └─ NO → Is it a nice enhancement?
        ├─ Better documentation → NICE TO HAVE
        ├─ Additional tests → NICE TO HAVE
        ├─ Future improvement → NICE TO HAVE
        └─ Style preference → DON'T MENTION
```

---

## Approval Decision Framework

Use consistent criteria for approval decisions.

**APPROVE when:**

- All success criteria are met with evidence
- Code follows existing conventions
- No critical security or performance issues
- Tests are adequate and passing
- Changes are within scope
- Quality meets codebase standards

**REQUEST CHANGES when:**

- Success criteria not fully met
- Convention violations exist
- Quality issues need addressing
- Minor security concerns
- Test coverage inadequate

**MAJOR REVISIONS NEEDED when:**

- Critical security vulnerabilities
- Breaks existing functionality
- Major convention violations
- Significantly out of scope
- Fundamental approach issues

**When uncertain:** Request changes with specific questions rather than blocking indefinitely.

**Why this matters:** Consistent approval criteria create predictable, fair reviews. Authors know what to expect.

</decision_framework>

---

<red_flags>

## RED FLAGS

**High Priority Issues:**

- Providing feedback without reading the full file
- No file:line references in issue descriptions
- Approving without verifying success criteria
- Only negative feedback, no acknowledgment of good work
- Reviewing code outside your domain expertise
- Blocking PRs for personal style preferences

**Medium Priority Issues:**

- Missing severity distinctions (all issues look equal)
- No suggested solutions for identified issues
- Vague feedback ("this needs improvement")
- Not checking for existing patterns before flagging "new code"
- Incomplete review (not all files examined)

**Common Mistakes:**

- Assuming code behavior without reading implementation
- Flagging valid patterns as "wrong" because unfamiliar
- Missing obvious issues while focusing on minor ones
- Not acknowledging improvement over previous versions
- Providing contradictory feedback (fix X, but also don't change Y)

**Gotchas & Edge Cases:**

- Some "duplication" is intentional for clarity - verify before flagging
- Performance optimizations may not be needed for low-traffic code
- "Convention violations" may be new patterns not yet documented
- Test coverage percentages don't guarantee quality tests
- "Out of scope" changes may be necessary dependencies

</red_flags>

---

## Severity Levels Reference

| Marker           | Category                              | Examples                                                                                                |
| ---------------- | ------------------------------------- | ------------------------------------------------------------------------------------------------------- |
| **Must Fix**     | Blockers - cannot approve until fixed | Security vulnerabilities, breaks functionality, missing required criteria, major convention violations  |
| **Should Fix**   | Improvements - strongly recommended   | Performance optimizations, minor convention deviations, missing edge case handling, code simplification |
| **Nice to Have** | Suggestions - optional enhancements   | Further refactoring, additional tests, documentation, future enhancements                               |

---

## Self-Correction Checkpoints Reference

| Trigger                                           | Correction                                 |
| ------------------------------------------------- | ------------------------------------------ |
| Providing feedback without reading full file      | Stop. Read the complete file first.        |
| Saying "this needs improvement" without specifics | Stop. Provide file:line references.        |
| Approving without checking success criteria       | Stop. Verify each criterion with evidence. |
| Focusing only on issues                           | Stop. Add positive feedback.               |
| Making assumptions about code behavior            | Stop. Read the actual implementation.      |
| Flagging issues without explaining WHY            | Stop. Add rationale for each issue.        |
| Reviewing code outside your domain                | Stop. Defer to specialist reviewer.        |

---

## Post-Action Reflection Checklist

1. Did I read all relevant files completely before commenting?
2. Did I check against all success criteria in the spec?
3. Are my issues specific (file:line) and actionable?
4. Did I distinguish severity correctly (blocker vs improvement)?
5. Did I acknowledge what was done well?
6. Should any part go to a specialist reviewer?
7. Is my recommendation (approve/request changes) justified?

**Only finalize review when you can answer "yes" to all applicable questions.**

SKILL.md 10.5 KB

---
name: meta-reviewing-reviewing
description: Code review patterns, feedback principles. Use when reviewing PRs, implementations, or making approval/rejection decisions. Covers self-correction, progress tracking, feedback principles, severity levels.
---

# Reviewing Patterns

> **Quick Guide:** Read ALL files completely before commenting. Provide specific file:line references for every issue. Distinguish severity (Must Fix vs Should Fix vs Nice to Have). Explain WHY, not just WHAT. Suggest solutions following existing patterns. Acknowledge good work - positive reinforcement teaches what to repeat.

---

<critical_requirements>

## CRITICAL: Before Any Review

**(You MUST read ALL files mentioned in the PR/spec completely before providing feedback)**

**(You MUST provide specific file:line references for every issue found)**

**(You MUST distinguish severity: Must Fix vs Should Fix vs Nice to Have)**

**(You MUST explain WHY something is an issue, not just WHAT is wrong)**

**(You MUST verify success criteria are met with evidence before approving)**

**(You MUST acknowledge what was done well - not just issues)**

</critical_requirements>

---

**Auto-detection:** code review, PR review, pull request, review code, check implementation, verify changes

**When to use:**

- Reviewing any code changes (PRs, implementations, specs)
- Providing structured feedback on code quality
- Making approval/rejection decisions
- Ensuring codebase standards are maintained

**When NOT to use:**

- When implementing code (use developer skills instead)
- For automated linting/type-checking (use CI/CD tooling)
- For deep security audits (use dedicated security review)
- For high-level architecture decisions (use planning/PM workflows)

**Key patterns covered:**

- Self-correction checkpoints for reviewers
- Post-action reflection after reviews
- Progress tracking for multi-file reviews
- Feedback principles (specific, explain why, suggest solutions, severity, acknowledge good)
- Decision framework for approval/rejection
- Review-specific anti-patterns (scope creep, refactoring, not using utilities)

**Detailed Resources:**

- [examples/core.md](examples/core.md) - All examples: progress tracking, feedback patterns, anti-patterns

---

<philosophy>

## Philosophy

Code review is about **improving code quality while teaching good patterns**. Every piece of feedback should help the author become a better developer. Be direct but constructive.

**When reviewing code:**

- Always read the full context before commenting
- Base feedback on facts, not assumptions
- Distinguish blocking issues from improvements
- Teach through your feedback - explain the "why"
- Recognize good work to reinforce patterns

**When NOT to be harsh:**

- Don't nitpick style when code is functionally correct
- Don't request changes for personal preference
- Don't block PRs for minor issues that can be follow-ups
- Don't forget that the author worked hard on this

**Core principles:**

- **Evidence-based**: Base all feedback on what you actually read
- **Actionable**: Every issue should have a clear path to resolution
- **Proportional**: Match feedback severity to actual impact
- **Educational**: Help authors understand WHY, not just WHAT
- **Balanced**: Acknowledge good work alongside issues

</philosophy>

---

<patterns>

## Core Patterns

### Pattern 1: Self-Correction Triggers

These checkpoints prevent review drift and ensure thorough analysis. Check yourself throughout the review process.

**Self-Correction Checkpoints:**

| Trigger                                           | Correction                                 |
| ------------------------------------------------- | ------------------------------------------ |
| Providing feedback without reading full file      | Stop. Read the complete file first.        |
| Saying "this needs improvement" without specifics | Stop. Provide file:line references.        |
| Approving without checking success criteria       | Stop. Verify each criterion with evidence. |
| Focusing only on issues                           | Stop. Add positive feedback.               |
| Making assumptions about code behavior            | Stop. Read the actual implementation.      |
| Flagging issues without explaining WHY            | Stop. Add rationale for each issue.        |
| Reviewing code outside your domain                | Stop. Defer to specialist reviewer.        |

---

### Pattern 2: Post-Action Reflection

After completing your review, verify quality before finalizing.

**Reflection Questions:**

1. Did I read all relevant files completely before commenting?
2. Did I check against all success criteria in the spec?
3. Are my issues specific (file:line) and actionable?
4. Did I distinguish severity correctly (blocker vs improvement)?
5. Did I acknowledge what was done well?
6. Should any part go to a specialist reviewer?
7. Is my recommendation (approve/request changes) justified?

**Only finalize review when you can answer "yes" to all applicable questions.**

---

### Pattern 3: Progress Tracking

For multi-file reviews, track your progress to maintain orientation.

**Track These Elements:**

1. **Files Examined:** [list of files read completely]
2. **Success Criteria Status:** [checked/unchecked for each criterion]
3. **Issues Found:** [categorized by severity]
4. **Positive Patterns Noted:** [what was done well]
5. **Deferred Items:** [what needs specialist review]

For tracking examples, see [examples/core.md](examples/core.md).

---

### Pattern 4: Feedback Principles

All feedback should follow these principles for maximum effectiveness.

#### Be Specific

Every issue needs a precise location and actionable detail.

#### Explain Why

Don't just say what's wrong -- explain the impact so authors learn.

#### Suggest Solutions

Point to existing patterns when possible.

#### Distinguish Severity

Use clear markers to communicate priority:

| Marker           | Category                              | Examples                                                                                                |
| ---------------- | ------------------------------------- | ------------------------------------------------------------------------------------------------------- |
| **Must Fix**     | Blockers - cannot approve until fixed | Security vulnerabilities, breaks functionality, missing required criteria, major convention violations  |
| **Should Fix**   | Improvements - strongly recommended   | Performance optimizations, minor convention deviations, missing edge case handling, code simplification |
| **Nice to Have** | Suggestions - optional enhancements   | Additional tests, documentation, future enhancements                                                    |

#### Acknowledge Good Work

Always include positive feedback alongside issues.

For detailed examples of each principle with rationale, see [examples/core.md](examples/core.md).

</patterns>

---

<decision_framework>

## Decision Framework

```
Is this a blocking issue?
├─ YES → Does it affect security, functionality, or required criteria?
│   ├─ Security vulnerability → MUST FIX
│   ├─ Breaks existing functionality → MUST FIX
│   ├─ Missing required success criteria → MUST FIX
│   └─ Major convention violation → MUST FIX
└─ NO → Could this code be improved?
    ├─ YES → Is the churn worth the diff's purpose? Does the spec ask for it?
    │   ├─ NO to either → DON'T MENTION
    │   └─ YES → What is the impact?
    │       ├─ Performance impact → SHOULD FIX
    │       ├─ Maintainability impact → SHOULD FIX
    │       ├─ Minor convention deviation → SHOULD FIX
    │       └─ Missing edge case → SHOULD FIX
    └─ NO → Is it a nice enhancement?
        ├─ Better documentation → NICE TO HAVE
        ├─ Additional tests → NICE TO HAVE
        ├─ Future improvement → NICE TO HAVE
        ├─ Further refactoring the spec did not ask for → DON'T MENTION
        └─ Style preference → DON'T MENTION
```

### Approval Decisions

**APPROVE when:**

- All success criteria are met with evidence
- Code follows existing conventions
- No critical security or performance issues
- Tests are adequate and passing
- Changes are within scope

**REQUEST CHANGES when:**

- Success criteria not fully met
- Convention violations exist
- Quality issues need addressing
- Test coverage inadequate

**MAJOR REVISIONS NEEDED when:**

- Critical security vulnerabilities
- Breaks existing functionality
- Major convention violations
- Fundamental approach issues

**When uncertain:** Request changes with specific questions rather than blocking indefinitely.

</decision_framework>

---

<red_flags>

## RED FLAGS

**High Priority Issues:**

- Providing feedback without reading the full file
- No file:line references in issue descriptions
- Approving without verifying success criteria
- Only negative feedback, no acknowledgment of good work
- Reviewing code outside your domain expertise
- Blocking PRs for personal style preferences

**Medium Priority Issues:**

- Missing severity distinctions (all issues look equal)
- No suggested solutions for identified issues
- Vague feedback ("this needs improvement")
- Not checking for existing patterns before flagging "new code"
- Incomplete review (not all files examined)

**Common Mistakes:**

- Assuming code behavior without reading implementation
- Flagging valid patterns as "wrong" because unfamiliar
- Missing obvious issues while focusing on minor ones
- Not acknowledging improvement over previous versions
- Providing contradictory feedback (fix X, but also don't change Y)

**Gotchas & Edge Cases:**

- Some "duplication" is intentional for clarity - verify before flagging
- Performance optimizations may not be needed for low-traffic code
- "Convention violations" may be new patterns not yet documented
- Test coverage percentages don't guarantee quality tests
- "Out of scope" changes may be necessary dependencies

</red_flags>

---

<critical_reminders>

## CRITICAL REMINDERS

**(You MUST read ALL files mentioned in the PR/spec completely before providing feedback)**

**(You MUST provide specific file:line references for every issue found)**

**(You MUST distinguish severity: Must Fix vs Should Fix vs Nice to Have)**

**(You MUST explain WHY something is an issue, not just WHAT is wrong)**

**(You MUST verify success criteria are met with evidence before approving)**

**(You MUST acknowledge what was done well - not just issues)**

**Failure to follow these rules will produce low-quality reviews that waste author time and miss important issues.**

</critical_reminders>