diff options
| author | Craig Jennings <c@cjennings.net> | 2026-05-25 15:11:08 -0500 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-05-25 15:11:08 -0500 |
| commit | a5b955f7c74ad5e27bb73c6d2143359cc66b2455 (patch) | |
| tree | 9bb2bced6576e53ded3a615aee5404ef55f7a610 /review-code | |
| parent | d27d07e8e4b004926ea960c118fa99caa979caa0 (diff) | |
| download | rulesets-a5b955f7c74ad5e27bb73c6d2143359cc66b2455.tar.gz rulesets-a5b955f7c74ad5e27bb73c6d2143359cc66b2455.zip | |
docs(skills): add voice pattern 40, praise/correction asymmetry
Voice gains pattern #40: strip the "why" from praise on an approve, since the author already knows why their change is good and the justification reads as flattery. Always keep the why on a finding or change-request, delivered gently. Behavior only changes when the reason lands.
review-code now runs a praise/correction gate before posting any summary, and its inline-comment guidance is tightened so the why-it-matters survives the brevity cuts. The reviewer states the stakes (a user hits a 500, a screen reader announces nothing), not just the mechanism.
Diffstat (limited to 'review-code')
| -rw-r--r-- | review-code/SKILL.md | 18 |
1 files changed, 12 insertions, 6 deletions
diff --git a/review-code/SKILL.md b/review-code/SKILL.md index 3e63b4a..78c5486 100644 --- a/review-code/SKILL.md +++ b/review-code/SKILL.md @@ -200,6 +200,8 @@ Remaining issues get tagged: ## Phase 5 — Output +Before printing any approve/request-changes summary for posting, run the praise/correction gate (see Posted Summary Voice, and `/voice` personal #40): scan the summary and cut every clause that describes or justifies a good change — keep praise plus verdict only. Then confirm each finding and change-request states its why, gently and briefly. This is a mechanical pass, not a judgment call. + ```markdown # Code Review — <PR title / branch name / SHA range> @@ -372,8 +374,10 @@ Name the good thing and stop: do not explain *why* it's good. The author made th This holds for re-review approvals too. A re-review confirming requested changes is just "Approving." Mechanical rule: an approve summary is the verdict plus at most a bare positive ("Clean.", "Solid fix."). It must contain no clause that says what the change does or why it works. "The hoist to App fixes the crash, and the new test locks it in" is the banned pattern — it describes and justifies the change on an approve. If a clause references the code's behavior, cut it. +The asymmetry: praise gets no why, but a finding, change-request, or inline coaching note *always* gets the why. Behavior only changes when the reason lands, so a correction that just says what to fix without saying why teaches nothing. Deliver that why gently and briefly, the way a kind coach would, never as a verdict from on high. The praise-strips / correction-explains split is enforced as `/voice` personal pattern #40, which every posted review summary passes through. + Good: -- "Nice, clean, good coverage. One small naming nit inline. Approving." +- "Nice, clean, good coverage. One small naming point inline. Approving." - "Clean shape, tests cover the right edges. Approving." - "Solid. One blocker inline — see the auth gap. Request changes." @@ -384,21 +388,23 @@ If specific praise lands somewhere, surface it as a single inline comment on the ## Inline Comment Voice -Inline pins are where the explanation lives, and they double as teaching opportunities. Write them in complete sentences and natural prose — the way a senior dev assigned to mentor a less-senior teammate would write them, not as compressed lint warnings or telegraphic fragments. Aim for around four sentences: what the problem is, when it would surface (or why it matters), what to do about it, and a generalizable lesson or framing that helps the reader build the mental model. Plain language; no jargon strings like "raises X -> 500 in case Y." +Inline pins are where the explanation lives. Write them in complete sentences and natural prose — the way a senior dev mentoring a less-senior teammate would, not as compressed lint warnings or telegraphic fragments. State three things: the problem, why it matters, and the fix — usually one to two sentences. The why-it-matters is not optional and is not the same as the mechanism: say what actually breaks and who it hurts (a user hits a 500, a screen reader announces nothing, the next caller silently gets a wrong default), not just what the code does. Keep it brief, but brevity never costs the stakes. Don't pad to a sentence quota, and skip the obligatory "the general lesson:" closer and the "when it would surface" elaboration unless that detail is itself substantive. Plain language; no jargon strings like "raises X -> 500 in case Y." (Craig, 2026-05-25: trimmed inline length twice, then flagged that the why-it-matters must stay in the short space — substance over length, stakes over mechanism.) -The summary body delivers the verdict in one line. The inline is the coaching moment — concise but readable, and it should leave the reader with a better understanding of the failure mode, not just the patch. +The summary body delivers the verdict in one line. Every inline carries its own why-it-matters, briefly. It should leave the reader understanding what breaks, why it matters, and the fix, without padding. Good (natural, complete sentences, coaches the reader): -> This `next(...)` call doesn't have a default, so it raises `StopIteration` and surfaces as a 500 if no matching frame is found. The `is_complete` gate above keeps that unreachable in normal flow today, but it's a foot-gun if `frames.json` ever drifts out of sync with `MAX_FRAMES`, or if anyone manually resets `is_complete` without resetting the index. Switching to `next(..., None)` plus an explicit raise (or logged return) would make the failure mode obvious if those preconditions ever break. More broadly: when a constant in code has to stay in sync with a data file, an explicit assertion at the read site catches drift before it becomes a runtime error. +> This `next(...)` call has no default, so it raises `StopIteration` and surfaces as a 500 if no matching frame is found. The `is_complete` gate keeps that unreachable today, but `next(..., None)` plus an explicit raise would harden it against `frames.json` drifting out of sync with `MAX_FRAMES`. Bad (compressed, mashed jargon, no coaching): > `next(...)` without a default raises StopIteration -> 500 if frames.json ever drifts out of sync with MAX_FRAMES. The is_complete gate makes that unreachable in normal flow, so just a heads-up for a follow-up — `next(..., None)` with a clear raise/return would land cleanly. -Bad (too terse — leaves the reader to fill in the gaps): +Bad (brief, but drops the why-it-matters — states mechanism and fix, never the stakes): + +> `next(...)` has no default. The is_complete gate keeps that unreachable today, but `next(..., None)` plus an explicit raise would harden it. -> Raises `StopIteration` -> 500 if no matching frame. The is_complete gate keeps that unreachable today, but `next(..., None)` plus an explicit raise would harden it. +The fix above tells the reader what to change without ever saying a user would hit a 500. Brevity is fine; dropping the stakes is not. The Good version is just as short and still names the impact. ## Content scope |
