---
name: fix-pr
description: Critically assess PR review comments — verify, investigate ripple effects, then fix or reject with evidence
---

# PR Review Comment Assessment

Critically assess each PR review comment — verify the concern, investigate ripple effects, then accept with a fix or reject with evidence.

## Quick Reference

| Aspect      | Detail |
|-------------|--------|
| **Trigger** | Invoked on a PR with review comments to address |
| **Input**   | PR number/URL, diff, review comments, full repo access |
| **Output**  | Per-comment decision record, grouped fix plan, reply text per comment |
| **Scope**   | Only comments on the current PR; do not refactor beyond what comments require |

## Assessment Framework

For each comment, answer:

1. **Valid?** Read the actual code (not just the diff). Does the concern apply?
2. **Severity?** Bug > correctness > consistency > style. Style-only comments are low priority.
3. **What breaks if changed?** Trace callers, check tests, find ripple effects.
4. **Full context?** Reviewers see diffs, not full files. They may miss structural reasons.
5. **YAGNI?** "You should also handle X" — does X actually occur in this codebase?

### Verdicts

- **ACCEPT** — Valid concern, will fix. Evidence: the code has the described problem.
- **REJECT** — Invalid, unnecessary, or harmful to change. Evidence: why it doesn't apply.
- **INVESTIGATE** — Need more information. List exactly what to check.

## Investigation & Ripple Effect

When a comment is non-obvious or touches shared code:

1. Read the full file(s) — not just the diff context
2. Search for usages of the symbol/pattern under discussion
3. Check test coverage — does a test assert the current behavior?
4. Follow the dependency graph: who calls this? who imports this? is it a public API?
5. Check git blame — why is the code written this way?
6. If ripple effects are significant, include them in the fix plan

Then decide ACCEPT or REJECT with evidence. INVESTIGATE is not a final state.

## Grouping Strategy

| Group together | Keep separate |
|---|---|
| Same file | Unrelated files/areas |
| Coupled files (caller/callee, type/impl) | Cosmetic vs functional changes |
| Same logical concern | Independent features |

Address grouped comments in a single commit with a bullet list of changes.

## Output Template

For each comment, produce:

```
Comment: #ID by @reviewer on file:line
Verdict: ACCEPT | REJECT
Reasoning: [1-2 sentences with evidence]
Ripple effects: [list or "none"]
Group: [group-id or "standalone"]
```

### Reply Format

**ACCEPT:** "Fixed. [description of change]." or "Fixed in [file]. Also updated [related file] for consistency."

**REJECT:** "Investigated — [reason this doesn't apply]. The current implementation [explanation]." or "This is intentional: [reason]. Changing it would [consequence]."

**Grouped:** "Addressed these comments together in [commit]. Changes: [bullet list]."

## Worked Example

> **Review comment** by @alice on `src/config.ts:42`:
> "This should validate the input before passing it to `loadConfig`. What if `path` is undefined?"

**Investigation:**
1. Read `src/config.ts` — `loadConfig` is only called from `cli/init.ts` which already validates all args via Zod schema.
2. `grep` for other callers — none. Single call site.
3. `path` is typed `string` (non-optional) in the function signature; TypeScript enforces this at compile time.

**Decision:**
```
Comment: #12 by @alice on src/config.ts:42
Verdict: REJECT
Reasoning: `path` is typed as required `string` and the sole caller validates via Zod before invocation. Adding a runtime check duplicates the type system and the caller's validation.
Ripple effects: none
Group: standalone
```

**Reply:** "Investigated — `path` is a required `string` param and the only caller (`cli/init.ts:18`) validates all args through a Zod schema before this point. Adding a runtime check would duplicate both the type constraint and the caller's validation."

## Common Reviewer Mistakes

| Pattern | Response |
|---------|----------|
| Abstraction for code used once | YAGNI — one usage doesn't need a helper |
| Error handling for impossible states | Only validate at system boundaries |
| Style preference framed as correctness | If it works and is readable, style is preference |
| Patterns from a different language/codebase | Follow THIS codebase's conventions |
| "Security issue" without a specific vector | Ask for the specific attack scenario |
| "Add tests for X" when X is already tested | Verify coverage before accepting |
| Diff-only context (missing full file) | They may miss structural reasons for the code |

## MUST DO / MUST NOT DO

| MUST DO | MUST NOT DO |
|---------|-------------|
| Read full file before deciding on a comment | Accept comments without verifying the concern |
| Provide evidence for every ACCEPT and REJECT | Use performative agreement ("Great catch!", "You're right!") |
| Check ripple effects before accepting changes | Fix cosmetic comments before functional ones |
| Search for existing patterns before introducing new ones | Assume the reviewer has full context |
| Resolve every INVESTIGATE to ACCEPT or REJECT | Leave comments unaddressed |

## Final Checklist

Before submitting:

- [ ] Every comment has a verdict (ACCEPT or REJECT) with evidence
- [ ] No INVESTIGATE verdicts remain unresolved
- [ ] Ripple effects checked for every ACCEPT
- [ ] Related comments grouped; each group addressed in one commit
- [ ] Reply text is technical — no performative agreement
- [ ] Fix plan covers all ACCEPT verdicts, including ripple effects