Skip to content

Commit 7eefe1b

Browse files
authored
Merge pull request #44 from redhat-developer/pr-review-style-and-verification
rhdh-pr-review: natural comment style, stricter file verification
2 parents 5211d82 + b23482d commit 7eefe1b

1 file changed

Lines changed: 9 additions & 17 deletions

File tree

skills/rhdh-pr-review/workflows/review-code.md

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ Reviewers will produce false positives. Verify each finding against actual code
3838
**Drop any finding that:**
3939

4040
- References code that doesn't exist at HEAD
41+
- References files that are not in the PR's changed files list (check the context artifact's `files[]` — don't assume a file exists in the PR just because it exists on the branch)
4142
- Was already raised and resolved in `existing_comments` or `existing_reviews`
4243
- Misreads what the code actually does
4344
- Matches existing codebase conventions (the PR follows the project's style, not the reviewer's preference)
@@ -48,34 +49,25 @@ Reviewers will produce false positives. Verify each finding against actual code
4849
- Tested?
4950
- Anything from the issue's scope missing? (Author may be intentionally splitting work — note, don't block.)
5051

51-
Present verified findings and dropped findings (with reasoning) to the user before drafting.
52+
Present verified findings and dropped findings (with reasoning) to the user before drafting. Use a structured format here — categorized by type (suggestion, question, observation), with `file:line` references and short descriptions. This is for the user to scan and approve, not the final comment text.
5253

5354
## Step 4: Draft the review
5455

55-
### Top-level comment
56+
The posted review should read like a person wrote it, not a report generator. The structured presentation in Step 3 helps the user decide what to include; the actual GitHub comments use a different voice.
5657

57-
1. One short sentence acknowledging the work.
58-
2. Frame inline items: "A few questions inline, nothing blocking."
59-
3. Requirements coverage summary if issues were checked.
60-
4. Keep to 3–5 sentences total.
58+
### Top-level comment
6159

62-
Keep the opening acknowledgment to one short sentence. Longer praise reads as performative.
60+
Keep it short and direct — frame what the inline comments are about so the author knows the scope at a glance. Include a requirements coverage note if linked issues were checked. Skip performative praise; it reads as filler.
6361

64-
If `existing_reviews` shows you've already left a top-level comment on this PR, a new top-level comment is often unnecessary — consider posting only the inline findings to reduce noise. Use judgment: a follow-up summary may still be warranted if the scope of feedback changed significantly or the prior review was on a different revision.
62+
If `existing_reviews` shows you've already left a top-level comment on this PR, a new one is often unnecessary — consider posting only the inline findings. A follow-up summary is still warranted if the scope of feedback changed significantly or the prior review was on a different revision.
6563

6664
### Inline comments
6765

68-
Post one inline comment per medium-or-above finding — no artificial cap. After presenting the medium+ findings, list each nit/low item as a one-line bullet (`file:line — short description`) so the user can quickly scan and cherry-pick which to include. Never leave a comment just to show you noticed something.
69-
70-
Choose the right comment type:
66+
Post one inline comment per finding worth raising — no artificial cap. Never leave a comment just to show you noticed something. Not every finding needs the same weight — substantial issues get a full comment, nits can be grouped into a single comment as one-liners.
7167

72-
| Type | When | Example |
73-
|------|------|---------|
74-
| Code suggestion | Clear fix, small scope | Missing guard, warning log, docs wording |
75-
| Question | Design decision, tradeoff | "Is X intended, or would Y avoid Z?" |
76-
| Observation | Worth noting, not actionable | "Return type changed — callers are safe" |
68+
Write each comment as natural prose — a short paragraph explaining the issue and why it matters. Avoid bullet lists, bold headers, and over-structured formatting. Keep just enough information for the author to understand the problem and act on it. Code suggestions and code blocks are fine since they're functional, not formatting.
7769

78-
Assume deliberate choices. Ask why before suggesting alternatives. Explain reasoning only when the fix isn't obvious. Call out specific things done well — name the pattern or decision, not generic praise.
70+
Assume deliberate choices. Ask why before suggesting alternatives. Explain reasoning only when the fix isn't obvious. Call out specific things done well when genuine — name the pattern or decision, not generic praise.
7971

8072
**If nothing significant survives verification**, that's a valid outcome. Produce a short approving review. Don't manufacture issues.
8173

0 commit comments

Comments
 (0)