Improve: review-pr skill — mechanism brief, traceability, multi-source goal#973
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates the Changesreview-pr Skill Workflow Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the PR review skill instructions (SKILL.md) to refine the review process. Key additions include prioritizing user-stated goals over the PR body, introducing a "Mechanism Brief" and a "Goal-Method Traceability" table, adding detailed analysis criteria (such as stable-boundary discipline and error propagation), and establishing a verification step to filter out hallucinated external reviewer findings. The review comments suggest improvements to the Markdown formatting of a table and replacing shell-unsafe placeholders (like <N> and <path>) with safer variable or path placeholders to prevent literal execution errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/skills/review-pr/SKILL.md (1)
148-157: ⚡ Quick winPrefer “most authoritative intended goal” over “most ambitious” wording.
The “most ambitious / most authoritative” phrase can push reviewers toward scope inflation. Rephrase to prioritize intended/approved scope from authoritative sources, then treat broader claims as discrepancies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/review-pr/SKILL.md around lines 148 - 157, Update the guidance in the "Stated Goal" section by replacing the phrase "most ambitious / most authoritative" with "most authoritative intended goal" and adjust the following sentence to instruct authors to prioritize the intended/approved scope from authoritative sources, while explicitly recording broader claims as discrepancies; specifically edit the text that currently reads 'Write your Stated Goal section using **the most ambitious / most authoritative** source...' and ensure the notes about tracking narrower sources (the bullet points referencing user/PR/issue scope differences) remain but frame broader or inflated claims as discrepancies to track.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/review-pr/SKILL.md:
- Around line 116-127: Update the "Sources, in priority order" section so linked
issue/design doc text is higher precedence than the `/review-pr` invocation:
reorder the list so "Linked GitHub issue or design doc" appears above "User's
natural-language description in the `/review-pr` invocation" while keeping the
PR body and user input still present (user input should remain above PR body but
below the linked issue/design doc); modify the numbered items under the "###
Sources, in priority order" heading to reflect this new precedence and adjust
any accompanying wording that refers to the current order.
---
Nitpick comments:
In @.claude/skills/review-pr/SKILL.md:
- Around line 148-157: Update the guidance in the "Stated Goal" section by
replacing the phrase "most ambitious / most authoritative" with "most
authoritative intended goal" and adjust the following sentence to instruct
authors to prioritize the intended/approved scope from authoritative sources,
while explicitly recording broader claims as discrepancies; specifically edit
the text that currently reads 'Write your Stated Goal section using **the most
ambitious / most authoritative** source...' and ensure the notes about tracking
narrower sources (the bullet points referencing user/PR/issue scope differences)
remain but frame broader or inflated claims as discrepancies to track.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e49b55e-7bca-436e-9506-fff480679432
📒 Files selected for processing (1)
.claude/skills/review-pr/SKILL.md
…e goal Adds three steps that the bug-hunting checklist alone could not produce: - Step 4 reworked to accept stated goals from multiple sources, ranking user-provided description (in the /review-pr invocation) above the PR body. Catches the failure mode where authors silently narrow the goal in their own description. - Step 5.5 Mechanism Brief — required on PRs over ~500 lines. Forces the reviewer to demonstrate they understood the design (data structures, lifetime, concurrency, cross-boundary contracts) before judging it. - Step 5.7 Goal-Method Traceability table — every stated goal maps to a specific design choice and code location, marked Solid / Underspecified / Weak / Missing / Implicit. ❌ Missing and ➕ Implicit rows become Must-fix findings. - Feature checklist extended from 4 to 8 questions (stable-boundary discipline, error propagation paths, concurrency model with contention testing, alternatives considered). - Step 7 adds a "Verification before surfacing" subsection with concrete git commands to catch the most common external-reviewer hallucination (citing code outside the PR diff as a PR finding). - Step 8 output structure updated to require Mechanism Brief and Traceability sections, plus a dropped-finding audit trail for external reviewers. - Common Pitfall hw-native-sys#5 updated: never silently accept the PR body's framing when other goal sources are broader. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ChaoWao
force-pushed
the
improve/review-pr-skill
branch
from
f19c3d1 to
e54a499
Compare
2 participants
Summary
Hardens
.claude/skills/review-pr/SKILL.mdso a review on a largedesign-heavy PR cannot get away with bug-spotting alone.
Three structural additions, three sharpened existing steps:
(1) the user's natural-language description in the
/review-prinvocation, (2) a linked GitHub issue / design doc, (3) PR body, or
(4) commit messages. User description outranks PR body so the common
failure mode — author silently narrowing the goal in their own
description — gets surfaced as a finding instead of silently
inherited.
Forces the reviewer to lay out data structures, lifetime, concurrency
model, and cross-boundary contracts before judging. Skipping it on a
large PR makes the rest of the review unreliable.
goal to a specific design choice and code location, marked Solid /
Underspecified / Weak / Missing / Implicit. ❌ Missing and ➕ Implicit
rows are must-discuss before approval. Catches "the PR talks about X
but never builds X" and "the PR builds Y that nobody asked for".
(version fields, reserved-field validation, deprecation path), error
propagation across boundaries (string-matching error translations are
bug magnets), concurrency model with contention testing, and
alternatives considered.
catch the most common external-reviewer hallucination — citing code
outside the PR diff as a PR finding. Also requires recording dropped
findings in Step 8's output for audit.
Traceability sections, plus the dropped-finding audit trail.
framing when other goal sources are broader.
The skill grew from ~14.9 KB to ~22.0 KB (+242 / -21 lines).
Test plan
markdownlint-cli2passes (already verified locally viapre-commit).
/review-pr <PR> allon a large PR —confirmed against PR Add: L3/L2 host-device mapped region design #861 across three iterations during
development; each successive skill revision caught a class of
finding the prior revision missed.
🤖 Generated with Claude Code