From d5dab9b715339b4386b15387bda4fe52137f2dfa Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Wed, 13 May 2026 19:24:05 -0500 Subject: docs(review-code): frame code review as a coaching channel Coaching guidance was buried under Inline Comment Voice at the bottom of the skill, where it read as a tone afterthought rather than the skill's purpose. This change threads it through Intent, Strengths, the Important-issue shape, Critical Rules, and Anti-Patterns so each step reads as mentoring with audit as the method. --- .claude/commands/review-code.md | 56 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/.claude/commands/review-code.md b/.claude/commands/review-code.md index d6501d0..ff32a1e 100644 --- a/.claude/commands/review-code.md +++ b/.claude/commands/review-code.md @@ -7,6 +7,12 @@ disable-model-invocation: true Review code changes against engineering standards. Produce a structured report with strengths, per-criterion audit, severity-tagged issues, and an explicit verdict. +## Intent + +Code review is a coaching channel. Every finding is a teaching opportunity — the engineer reading the review should come away with a better mental model of the failure pattern, not just a list of patches to apply. The structured report below is one half of the work; the inline pins and posted summary on the PR are where the coaching actually lands. + +Reviews that block without teaching are incomplete. Reviews that teach without honest assessment are flattery. The aim is both: honest about the gaps, generous about what's right, and specific enough that the engineer can apply the lesson to the next PR. + ## Usage Point the skill at code being reviewed in any of these ways: @@ -204,7 +210,7 @@ Remaining issues get tagged: ## Strengths -Three minimum. Specific, with file:line. +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). - - @@ -230,12 +236,16 @@ Three minimum. Specific, with file:line. 1. **** - File: `<path>:<line>` - Problem: <what's wrong> - - Why it matters: <impact> + - Why it matters: <impact + generalizable principle> - Fix: <concrete suggestion> ### Important (should fix) -1. **<Title>** — `<file>:<line>` — <one-sentence description> +1. **<Title>** + - File: `<path>:<line>` + - Problem: <what's wrong> + - Why it matters: <impact + generalizable principle> + - Fix: <concrete suggestion> ### Minor (nice to have) @@ -249,7 +259,7 @@ Meta-level suggestions: process changes, follow-up tickets, architectural drift **<Approve / Request Changes / Needs Discussion>** -**Reasoning:** <1-2 sentences. Grounded in the audit.> +**Reasoning:** <1-2 sentences. Grounded in the audit. For Request Changes, name what would clear the verdict so the path to approval is visible.> ``` ## Critical Rules @@ -262,6 +272,8 @@ Meta-level suggestions: process changes, follow-up tickets, architectural drift - 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 +- Treat each finding as a coaching opportunity — the engineer reading should come away with a better mental model, not just a patch list +- Frame Request Changes as a path to approval — name what would clear it, not just what's broken **DON'T:** - Say "looks good" without citing what was checked @@ -340,6 +352,8 @@ None. - **Reviewing code you didn't read.** Don't feedback on files you skimmed. - **Running the build yourself.** CI does that. Don't re-verify what CI is for. - **Hedging ("might be an issue").** If you can't commit to it, it's Low-confidence — drop it. +- **Blocking without teaching.** A Request Changes verdict without coaching is incomplete. Name the principle behind the fix, not just the fix. +- **Coaching without honesty.** Inflating Strengths to soften a rough review reads as flattery. Honest assessment with generous framing beats puffed-up praise. ## Hand-Off @@ -348,6 +362,40 @@ None. - **Minor** → follow-up issues or a cleanup PR - **Intent-vs-Delivery gaps** → either file tickets for the missing pieces or update the plan to reflect reality +## Posted Summary Voice + +The summary body and the inline pins work as a pair: scannable verdict on top, full coaching conversation in the pins. Read this section paired with Inline Comment Voice below — the summary is terse precisely because the inlines carry the teaching weight. + +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. + +Good: +- "Nice, clean, good coverage. One small naming nit inline. Approving." +- "Clean shape, tests cover the right edges. Approving." +- "Solid. One blocker inline — see the auth gap. Request changes." + +Bad (chatty, padded, marketing-adjacent): +- "Great work overall! This is a really clean addition. The OneToOne relationship behaves as expected, the migration is correctly dependent on 0028, CI is green across all backend/frontend checks, and the tests cover Normal/Boundary/Error cases. One small naming nit inline — fine to roll into a follow-up." + +If specific praise lands somewhere, surface it as a single inline comment on the relevant line, not in the summary body. The summary stays scannable; the inline pins carry the specifics. + +## 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." + +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. + +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. + +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): + +> 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. + ## Content scope Output this skill produces that gets committed or shared with the team must follow the *Content scope for public artifacts* rule in [`commits.md`](../claude-rules/commits.md): no local paths, no private repo names, no personal tooling references. -- cgit v1.2.3