aboutsummaryrefslogtreecommitdiff
path: root/review-code
diff options
context:
space:
mode:
authorCraig Jennings <c@cjennings.net>2026-05-22 13:48:59 -0500
committerCraig Jennings <c@cjennings.net>2026-05-22 13:48:59 -0500
commita4389e8259a68d6e237485f6de4f05c7d1c6cc5d (patch)
tree054b9980829b135c8ca701ab23b712f45f73435e /review-code
parenta785f54eb4f8e45f97be737047eb5a4547a5dd5a (diff)
downloadrulesets-a4389e8259a68d6e237485f6de4f05c7d1c6cc5d.tar.gz
rulesets-a4389e8259a68d6e237485f6de4f05c7d1c6cc5d.zip
docs(skills): keep review-code praise honest and unforced
Two related changes to review-code's strengths guidance. The mandatory "three minimum" could force filler on a tiny diff or padded praise on a weak PR, so I relaxed it to up to three specific strengths, with an honest "nothing notable" allowed when the diff doesn't earn them. I also reframed the old "No Strengths section" anti-pattern as "skipping strengths out of laziness": a substantive diff still demands them, a weak one doesn't. The other change tells reviewers to name the good thing and stop, without explaining why it's good. Explaining praise reads as sycophantic since the author already knows the rationale. Elaboration is for findings, not compliments.
Diffstat (limited to 'review-code')
-rw-r--r--review-code/SKILL.md8
1 files changed, 5 insertions, 3 deletions
diff --git a/review-code/SKILL.md b/review-code/SKILL.md
index 84a28e5..7a1d175 100644
--- a/review-code/SKILL.md
+++ b/review-code/SKILL.md
@@ -210,7 +210,7 @@ Remaining issues get tagged:
## Strengths
-Three minimum. Specific, with file:line. Strengths name what to repeat — they're as much coaching as the issues list. Call out the things the engineer did well that you want them doing on the next PR (good test structure, clean separation of concerns, root-cause fix rather than symptom patch).
+Up to three, each specific and with file:line. Strengths name what to repeat — they're as much coaching as the issues list. Call out the things the engineer did well that you want them doing on the next PR (good test structure, clean separation of concerns, root-cause fix rather than symptom patch). On a tiny diff, one or two honest strengths beat three padded ones; on a genuinely weak PR, "nothing notable here" is a fair finding — say it rather than manufacture praise.
- <Specific positive>
- <Specific positive>
@@ -268,7 +268,7 @@ Meta-level suggestions: process changes, follow-up tickets, architectural drift
- Categorize issues by actual severity — not everything is Critical
- Cite specifics — `file:line`, not vague prose
- Explain why each issue matters, not just what it is
-- Acknowledge strengths — mandatory; three minimum
+- Acknowledge real strengths — up to three, specific; say none found when a tiny or weak diff doesn't warrant them
- Give a clear verdict with reasoning
- Trust CI for lint, typecheck, test runs; don't re-run them
- Self-filter low-confidence findings before reporting
@@ -346,7 +346,7 @@ None.
## Anti-Patterns
- **All-caps everything.** If everything is Critical, nothing is. Be honest about severity.
-- **No Strengths section.** Reviews that only list problems are inaccurate — you missed what's right. Minimum three.
+- **Skipping strengths out of laziness.** On a substantive diff, if you found nothing good you didn't read carefully — name up to three specifics. The exception is a tiny or genuinely weak diff, where an honest "nothing notable" is the right call and padding would be synthetic praise.
- **Verdict without reasoning.** "Request Changes" alone is unhelpful. One or two sentences on why.
- **Criteria skipped silently.** If you didn't check API contracts, say "N/A — no API changes in this diff," not silence.
- **Reviewing code you didn't read.** Don't feedback on files you skimmed.
@@ -368,6 +368,8 @@ The summary body and the inline pins work as a pair: scannable verdict on top, f
The structured report above stays local. When the verdict is posted as a GitHub review (per `commits.md` Step 2 Shape 1), keep the summary body terse — one long sentence or a few short ones is plenty. Vary the phrasing run-to-run so consecutive reviews don't read templated. Voice: an encouraging senior dev who doesn't like to talk; positive feedback is short, blunt, and lands cleanly.
+Name the good thing and stop: do not explain *why* it's good. The author made the change and already knows the rationale, so justifying the praise reads as sycophantic. "Clean migration off the window globals, tests cover the right edges" lands; appending "...because there are no stragglers and the provider, mocks, and Normal/Boundary/Error cases are all covered" turns a compliment into padding. Elaboration is for findings (something is wrong, here's the failure mode and the fix), never for compliments.
+
Good:
- "Nice, clean, good coverage. One small naming nit inline. Approving."
- "Clean shape, tests cover the right edges. Approving."