From 043a2948768b95633647c881759e66306ffab4d9 Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 27 Jul 2026 17:04:51 +0200 Subject: [PATCH] claude-plugins: move pr-review skill into base, not its own plugin --- .../plugins/base/skills/pr-review/SKILL.md | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 claude-plugins/plugins/base/skills/pr-review/SKILL.md diff --git a/claude-plugins/plugins/base/skills/pr-review/SKILL.md b/claude-plugins/plugins/base/skills/pr-review/SKILL.md new file mode 100644 index 00000000..17e5e394 --- /dev/null +++ b/claude-plugins/plugins/base/skills/pr-review/SKILL.md @@ -0,0 +1,90 @@ +--- +name: pr-review +description: Generic mechanics and conventions for reviewing a pull request - label every finding with a severity (block/note/good), don't approve with an unresolved one-line fix outstanding, check correctness/security/consistency/tests/docs, resolve any opaque reference before approving, and submit a formal approve/request-changes verdict rather than a plain comment. Use this whenever you're about to review someone else's pull request, from first read to submitting your verdict. +--- + +# PR Review + +A review's job is to catch what the author missed, not to rewrite their +work in your own style. That framing should hold throughout. + +## Before you start + +1. **Read the linked issue/task** - understand *why* the change exists, + not just what it does. +2. **Check CI status first.** A failing build or test suite is an + automatic block - don't write a detailed review of code that doesn't + even pass its own checks. Unblock the build first. +3. **Pull the full source when context matters**, not just the diff - + diffs lie by omission. + +## Every finding needs a severity + +- **block** - must fix before merge: a bug, a security issue, broken + behavior, a failing test, a hardcoded value that will rot. +- **note** - worth fixing, author's call: an inconsistency, a missing + edge case, a stale comment, copy-paste smell. +- **good** - explicitly call out what's done well (optional, but + appreciated - a review that's only ever critical trains authors to + dread review instead of valuing it). + +Mixing blocks and notes without labeling them forces the author to +guess what's actually required before merge. + +## Don't approve with an outstanding one-line fix + +If a finding is a trivial fix, it belongs in the PR before merge - don't +approve with it left as a dangling nit. Request changes when the fix is +that cheap. Approving with unresolved issues trains authors to treat +review comments as optional. + +## What to check + +- **Correctness** - does it do what the issue says? Handle edge cases + (empty input, off-by-one, concurrent access)? Any silent failure paths + (swallowed errors, unchecked returns)? +- **Security** - credentials/tokens ever logged or echoed? User-supplied + input injected into a shell command, query, or DOM? File paths + validated before use? +- **Consistency** - do new identifiers match the surrounding file's + conventions? Same error-handling pattern as neighboring code? Doc + comments in the same style and tense as existing ones? +- **Tests** - do they exercise the actual new behavior, including + failure paths, not just the happy path? Are names descriptive enough + to diagnose a failure without reading the body? +- **Documentation** - does any public interface, flag, or config option + need a doc update? Are stale docs cleaned up alongside the change? + +## Opaque references + +Any comment or identifier that references something by name (a ticket +number, an internal codename, a `NOTE(foo)`-style marker) is only useful +if it's actually resolvable. Search for the referenced thing before +approving; if you can't resolve it, flag it - missing context isn't a +reason to wave a change through. + +## Submit a formal verdict, not just a comment + +A plain comment does **not** set a PR's review state and won't +block or unblock a merge on most forges. Every review should end with +an explicit **approve** or **request-changes** verdict through +whatever formal review mechanism your forge tooling exposes - not just +prose in a comment that looks like a verdict but doesn't function as one. + +## Format + +Keep it tight - one finding per bullet, each labeled with its severity, +two sentences max per finding. If a finding needs more than that, it's +architectural enough to be its own comment, not a review bullet. + +## Tone + +Precise, not harsh. "This will panic on an empty slice" is useful. "This +is bad" is not - point at the exact spot, describe the failure mode, +suggest the fix. Don't soften a real block into a vague nit to be nice; +a watered-down block is just a block that slips into production. + +If your project ships its own domain-specific review checklist (gates +that differ by author/area, project-specific tooling commands), treat +that as the canonical addition on top of this - project specifics don't +belong duplicated in here.