Good Pr

Good PR

A skill for crafting pull requests that respect maintainers' time and earn trust. Every piece of guidance here comes from inverting real maintainer frustrations — these are the things that separate contributions that get merged from ones that get closed.

Why This Matters

Maintainers have to own every line of code that gets merged. If your PR introduces a subtle bug and you disappear for a month (which is completely fine — OSS contributors have lives), the maintainers are the ones debugging it at 2am. A PR that looks "80% done" can still be a net negative if the remaining 20% is a landmine.

Your job as a contributor is to make the maintainer's life easier, not harder. That means doing the work to prove your change is correct — not just asserting it. The checklist below is how you do that.

Quick Navigation

What does the user need help with?

├─ Writing a PR description     -> references/pr-template.md (blank template)
│                                  references/pr-example.md (filled-in example)
├─ Visual change or screenshot  -> references/visual-evidence.md
│  evidence review                 scripts/check-visual-evidence.py
├─ Self-reviewing before submit -> references/review-checklist.md
├─ Automated readiness check    -> scripts/check-pr-readiness.py
├─ Understanding what makes
│  a good PR                    -> The Checklist (below)
└─ Common mistakes to avoid     -> Anti-Patterns (below)

The Checklist

Walk through each area when preparing a PR. Not every PR needs every item, but every item should be consciously considered.

1. Reproduction Steps

A maintainer who can't reproduce a bug from your description will close the PR — not because they're dismissive, but because they literally cannot verify the fix.

The PR should reference a specific issue. If none exists, create one. Include:

• Environment details (OS, runtime version, browser, etc.)
• Exact steps to trigger the bug
• What currently happens (broken behavior)
• What should happen instead

Example:

## Steps to reproduce
1. Run `npm start` with Node 20.11
2. Navigate to /settings/profile
3. Click "Save" without changing any fields
4. Observe: 500 error in console — expected: no-op or "no changes" toast

If you can't reproduce the bug reliably, you can't prove your fix works.

2. Visual Evidence

Any PR with visible UI or rendered-output impact must include reviewable visual evidence. A changed UI file is only a signal: if the pixels genuinely do not change, say why instead of manufacturing screenshots. Asking maintainers to pull your branch and click around is asking them to do your job.

• Ordinary UI: a hand-taken, captioned screenshot is sufficient for a static change; use a recording for interaction or animation. Cover only the states and viewports needed for the review decision, and do not demand generated artifacts or a render pipeline for a trivial UI change.
• Programmatically generated output: read references/visual-evidence.md before drafting or reviewing the evidence. Prefer artifacts from the production renderer to hand-captured demo output. Apply the compact contract: one visible claim; the same named input at immutable base/head revisions; focused, captioned before/after output with full-SHA URLs, descriptive alt text, and a reviewer cue; a contact sheet for large matrices; a reproduction command or receipt; an existing independent check when machine-verifiable; and an explicit limitation.
• New or previously unsupported output: preserve the honest error or unsupported baseline; never fabricate a before image.
• No visible impact: state briefly why pixels do not change and provide the non-visual evidence that applies.

Keep proof proportional. Reuse the production renderer, checked-in fixtures, and existing tests or metrics; avoid parallel rendering logic or proof-only approval infrastructure unless recurring risk justifies its maintenance.

After drafting a PR with visual impact, save the description to a Markdown file and run the installed linter. Resolve the script relative to this SKILL.md; do not assume the target repository has copied it into its own scripts/ directory:

python3 <good-pr-skill-dir>/scripts/check-visual-evidence.py --kind ui /tmp/pr-body.md
python3 <good-pr-skill-dir>/scripts/check-visual-evidence.py --kind generated /tmp/pr-body.md

Use ui for ordinary application screenshots and generated for renderers, charts, PDFs, image pipelines, or other programmatic output. The linter checks only mechanical properties: section/media presence, malformed Markdown, alt/link-text presence, immutable repository URLs, and labelled base/head SHAs. It ignores non-rendered Markdown examples and never decides whether claims, fixtures, receipts, tests, limitations, or pixels are valid. Review warnings in context; use --strict only when the target project explicitly treats every mechanical warning as blocking. Prefer the readiness wrapper for generated evidence because it supplies the actual merge-base and HEAD; direct mode checks only SHA shape.

3. Code That Fits

Study the codebase before contributing. A PR that "works" but requires a full refactor before merging is one the maintainer will rewrite from scratch. Match the project's patterns:

• Naming conventions, file structure, abstraction style
• Error handling patterns already in use
• No new dependencies without prior discussion
• No unrelated refactoring mixed into the PR
• Minimal diff — solve exactly the problem described

If you think the codebase should be structured differently, open a discussion first. Don't smuggle an architecture change into a bug fix.

4. Tests That Prove the Fix

This is where contributions most often quietly fail. A test suite that passes is not evidence of correctness — your tests must fail when the bug is reintroduced. This is the litmus test:

1. Write the test
2. Confirm it passes with your fix
3. Revert your fix temporarily
4. Confirm the test fails
5. Re-apply the fix

If a test passes both with and without the fix, it tests nothing. Maintainers will check this — they'll reintroduce the bug to see if your test catches it. If it doesn't, the PR loses all credibility.

For security, authorization, data-exposure, or permission bugs, make the evidence especially concrete without turning it into checklist theater. Ask for one targeted negative regression test that asserts the security invariant and would fail on the vulnerable or reverted code. Good evidence usually includes:

• The denied path: 401/403, thrown auth error, or blocked side effect
• No protected data leaked and no unauthorized state change performed
• A positive control showing the authorized path still works
• A short PR testing note with the command run and "verified this test fails when the fix is reverted" — or a brief reason if that check is not practical

Do not ask for broad security theater: no full exploit write-up, screenshots, or unrelated suite expansion unless the project asks for it. Keep the ask scoped to evidence the maintainer can review quickly.

Mention this verification in the PR description:

## Testing
- Added unit test for empty-form submission in `settings.test.ts`
- Verified test fails when the guard clause on line 42 is removed
- Existing test suite passes (no regressions)

5. Scoped and Safe

A PR that claims to fix one thing but touches many things is a red flag. Even if each individual change is correct, the combined risk multiplies.

• Keep PRs small and focused on a single concern
• Run the full test suite, not just your new tests
• Note areas of risk in the description ("this changes the auth middleware, so I also manually tested login and signup flows")
• If your fix touches shared code, explain why and what you verified

Don't self-assign emergency labels or demand rollbacks unless you have clear evidence of widespread breakage or the project asks contributors to flag severity. For security fixes, use a neutral, specific title, state concrete impact/evidence, and let maintainers choose urgency and triage. Panic PRs that break more than they fix destroy trust faster than anything.

6. A Description That Stands Alone

The PR description is your pitch. A maintainer should understand everything about this change without pulling the code.

See references/pr-template.md for a fill-in template. The key sections:

• What — one-sentence summary
• Why — link to issue, brief root cause explanation
• How — approach taken, alternatives considered
• Testing — what you tested, how, and evidence it works
• Risk — what could go wrong and how you mitigated it

7. Trust Is Earned

If this is your first contribution to a project:

• Start small — fix a typo, improve docs, tackle a "good first issue"
• Be responsive to review feedback (don't ghost after submitting)
• Accept that maintainers may rewrite parts of your code
• Don't argue style if the project has established conventions

Maintainers remember reliable contributors. A few solid small PRs buy you latitude for bigger changes later.

Anti-Patterns

These are real mistakes that get PRs closed. Each shows the wrong way and the right way — learn from the contrast.

Bad vs. Good PR Description

# NEVER — vague, no context, no evidence

Fix login bug

Changed the form to use onSubmit instead of onClick.

# ALWAYS — specific, linked, proven

Fix crash on empty form submission (#247)

## What
Fix 500 error when saving settings with no changes.

## Why
Fixes #247. `updateSettings()` sends an empty PATCH body that the API
rejects. ~300 errors/day in Sentry since 2.4.0.

## How
Added early return in `handleSubmit()` when no fields changed.

## Testing
- Added test in `settings.test.ts`
- Verified test fails when guard clause is removed
- Full suite passes (247/247)

Bad vs. Good Tests

// NEVER — passes with or without the fix (tests nothing)
test('should handle invalid tokens', () => {
  const result = validateToken('bad-token');
  expect(result).toBeDefined();
});

// ALWAYS — fails when the fix is reverted (proves the fix)
test('should reject expired tokens with TokenExpiredError', () => {
  const expired = createToken({ exp: Date.now() - 1000 });
  expect(() => validateToken(expired)).toThrow(TokenExpiredError);
});

Bad vs. Good Scope

# NEVER — "drive-by refactor" mixed into a bug fix
- const x = getData()
+ const userData = getUserData()  // renamed for clarity
+ // also reformatted this whole file with prettier
+ // also upgraded lodash while I was in here

# ALWAYS — minimal diff, solves exactly one problem
- <div className="login-form">
+ <form className="login-form"

How to Use This Skill

When a user asks for help with a PR:

1. Understand the context — ask what project this is for, whether they've contributed before, and what the change does
2. Review the diff — if they share code, check it against section 3 (code fit) and section 5 (scope). Flag unrelated changes or pattern mismatches
3. Check for gaps — walk through the checklist and flag what's missing. Use references/review-checklist.md as a quick-scan tool
4. Help write the description — use references/pr-template.md as a starting point and fill it in together
5. Probe the tests — if they have tests, ask: "does this test fail when you revert the fix?" If they haven't checked, tell them to check
6. Visual changes? — classify the change as ordinary UI, generated output, a new/unsupported surface, or genuinely no visible impact. Read references/visual-evidence.md, run scripts/check-visual-evidence.py with the matching --kind, then apply the semantic and proportionality guidance that the mechanical linter cannot judge
7. Be honest — it's better to tell someone their PR needs work before they submit it than to let them waste a maintainer's time and damage their reputation in the project