From e35fe600ef9ec3bf2facae67608b0a8bf0298ed9 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sun, 19 Apr 2026 16:28:03 -0500 Subject: refactor: review-pr → review-code with superpowers + plugin-lifted improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renamed review-pr → review-code (the skill accepts PR, SHA range, current branch, staged changes — "pr" was understating scope). Rewrote SKILL.md with YAML frontmatter (previously header-style) and merged useful patterns from two sources: From obra/superpowers skills/requesting-code-review: - Intent-vs-delivery grading (given plan/ADR/ticket) - Mandatory Strengths section (three minimum) - Per-issue Critical/Important/Minor severity (per-criterion PASS/WARN/FAIL retained; complementary axes) - Required verdict + 1-2 sentence reasoning - Multi-input support (PR / SHA range / current branch / --staged) - Sub-agent dispatch recommendation for heavy reviews - Concrete filled-in example output From the claude-plugins-official code-review plugin: - Phase 0 eligibility gate (skip closed/draft/auto/trivial/already-reviewed) - CLAUDE.md traversal + adherence criterion (reads root + per-directory CLAUDE.md files; audits the diff against stated rules) - Multi-perspective Phase 2: five passes (CLAUDE.md adherence, shallow bug scan, git history context, prior PR comments, in-scope code comments). For large reviews, dispatch as parallel sub-agents. - Confidence filter (High/Medium/Low; drop Low before reporting) - False-positive categories explicitly enumerated (pre-existing issues on unmodified lines, lint/typecheck issues CI handles, senior-wouldn't-call-out nitpicks, silenced issues with valid reason, intentional scope changes, unmodified-line issues, framework-behavior tests) - Trust-CI discipline (don't run builds yourself) Substance from the original review-pr kept verbatim: - DeepSat-specific criteria (security, TDD evidence, conventions, no-AI-attribution, API contracts, architecture layering, root-cause discipline) Size: 60 lines → 347 lines. Growth is structural (added phases, added example, added perspectives, added filters) not verbose — each section earns its lines. NOT adopted from the plugin: - GitHub comment output format (plugin posts PR comments; review-code outputs a markdown report the user can paste if they want) - "Generated with Claude Code" footer (violates no-AI-attribution rule) - Specific 0/25/50/75/100 confidence scale (Critical/Important/Minor covers the same signal with less ceremony) Makefile SKILLS updated: review-pr → review-code. Old ~/.claude/skills/review-pr symlink removed; make install creates the new one at ~/.claude/skills/review-code. --- review-pr/SKILL.md | 57 ------------------------------------------------------ 1 file changed, 57 deletions(-) delete mode 100644 review-pr/SKILL.md (limited to 'review-pr/SKILL.md') diff --git a/review-pr/SKILL.md b/review-pr/SKILL.md deleted file mode 100644 index c7ebd75..0000000 --- a/review-pr/SKILL.md +++ /dev/null @@ -1,57 +0,0 @@ -# /review-pr — Review a Pull Request - -Review a PR against engineering standards. - -## Usage - -``` -/review-pr [PR_NUMBER] -``` - -If no PR number is given, review the current branch's open PR. - -## Instructions - -1. Fetch the PR diff and description using `gh pr view` and `gh pr diff`. -2. Review against these criteria, reporting each as PASS / WARN / FAIL: - -### Security -- No hardcoded secrets, tokens, API keys, or credentials -- All user input validated at system boundary -- Parameterized queries only (no SQL string concatenation) -- No sensitive data logged (PII, tokens, passwords) -- Dependencies pinned and auditable - -### Testing (TDD Evidence) -- Tests exist for all new code — check for test commits **before** implementation commits (TDD workflow) -- All three categories covered: normal (happy path), boundary, and error cases — edge cases must be thorough, not token -- Tests are independent — no shared mutable state between tests -- Mocking is at external boundaries only (network, file I/O, time) — domain logic tested directly -- Test naming follows project convention -- Coverage does not decrease — flag PRs that lower coverage without justification - -### Conventions -- Type annotations on all functions (including return types) -- Conventional commit messages (`feat:`, `fix:`, `chore:`, etc.) -- No AI attribution anywhere (code, comments, commits, PR descriptions) -- One logical change per commit -- Docstrings on public functions/classes - -### Root Cause & Thoroughness -- Bug fixes address the root cause, not surface symptoms — if the fix is a band-aid, flag it -- Changes demonstrate understanding of the surrounding code (not just the changed lines) -- Edge cases are covered comprehensively, not just the obvious happy path - -### Architecture -- Request handlers deal with request/response only — business logic in services or domain layer -- No unnecessary abstractions or over-engineering -- Changes scoped to what was asked (no drive-by refactoring) - -### API Contracts -- New endpoints have typed contracts or schemas defined -- No raw dict/object responses bypassing the contract layer -- Client-side types match server-side output -- Data flows through the API layer, not direct data access from handlers - -3. Summarize findings with a clear verdict: **Approve**, **Request Changes**, or **Needs Discussion**. -4. For each WARN or FAIL, include the file and line number with a brief explanation. -- cgit v1.2.3