diff options
Diffstat (limited to 'review-code')
| -rw-r--r-- | review-code/SKILL.md | 8 |
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." |