aboutsummaryrefslogtreecommitdiff
path: root/review-code/SKILL.md
diff options
context:
space:
mode:
Diffstat (limited to 'review-code/SKILL.md')
-rw-r--r--review-code/SKILL.md23
1 files changed, 13 insertions, 10 deletions
diff --git a/review-code/SKILL.md b/review-code/SKILL.md
index ec08a9d..3757404 100644
--- a/review-code/SKILL.md
+++ b/review-code/SKILL.md
@@ -273,9 +273,9 @@ Remaining issues get tagged:
## Phase 5 — Output
-**Terminal display — plain text only.** Everything this skill echoes to Craig in the chat terminal — the structured report, the per-criterion table, the verdict, and any draft summary or inline awaiting approval — must be rendered as plain text. No bold (`**`), no backtick code spans, no markdown tables, no headings-as-markup. They render as reverse video in his terminal and are hard to read. Write `file.py:42` as bare text, identifiers and `test:`-style prefixes unquoted, the criterion audit as a plain dashed list rather than a table, severity tiers as plain labels. This applies only to the terminal echo. The review actually posted to GitHub (the `gh api` body and inline blocks) keeps normal markdown — GitHub renders it correctly, so draft the posted artifact in markdown and strip the markup only when mirroring it into the chat. (Same rule as `interaction.md`'s no-reverse-video constraint; repeated here because the violation happens at exactly this print step.)
+**Terminal display — plain text only.** Everything this skill echoes to Craig in the chat terminal — the structured report, the per-criterion table, the verdict, the draft summary, and the full prose of every inline pin awaiting approval (the exact words, never a description of them) — must be rendered as plain text. No bold (`**`), no backtick code spans, no markdown tables, no headings-as-markup. They render as reverse video in his terminal and are hard to read. Write `file.py:42` as bare text, identifiers and `test:`-style prefixes unquoted, the criterion audit as a plain dashed list rather than a table, severity tiers as plain labels. This applies only to the terminal echo. The review actually posted to GitHub (the `gh api` body and inline blocks) keeps normal markdown — GitHub renders it correctly, so draft the posted artifact in markdown and strip the markup only when mirroring it into the chat. (Same rule as `interaction.md`'s no-reverse-video constraint; repeated here because the violation happens at exactly this print step.)
-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.
+Before posting, print the full summary body AND every inline comment exactly as it will post — never the summary alone with an inline merely described ("I'd pair it with one inline on..."). Craig approves the exact words that post under his name, so the exact words must be on screen. Then run the praise/correction gate (see Posted Summary Voice, and `/voice` personal #40): scan the summary and cut every praise clause and every clause that describes or justifies a good change — an approve summary is the substantive pointer plus the verdict, no praise, not even a bare positive. 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>
@@ -446,21 +446,24 @@ None.
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.
+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. Lead with the substantive pointer — the design note or blocker that's pinned inline — and close with the verdict; the summary carries no praise clause.
-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.
+The summary body carries no praise — not a named good thing, not a bare positive. The author made the change and already knows its merits, so a compliment in a terse summary reads as filler or sycophancy. If a genuine positive is worth surfacing, it goes as a single inline pin on the relevant line (see below), never the summary body. Elaboration in the summary is for the substantive pointer and for findings — what's wrong, the failure mode, the fix — never for compliments.
-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.
+This holds for re-review approvals too. A re-review confirming requested changes is just "Approving." Mechanical rule: an approve summary is the substantive pointer (the inline design note, if any) followed by the verdict — no praise, not even a bare positive. Lead with the pointer, close with the verdict: "One design note inline, not a blocker. Approving." An approve with nothing to flag is just "Approving." No clause may say what the change does or why it works, and none may compliment it — "Clean.", "Solid fix.", "The hoist to App fixes the crash and the new test locks it in" are all cut. If a clause references the code's behavior or praises it, 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.
+The asymmetry: the summary drops praise entirely, 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 drop-praise / 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 point inline. Approving."
-- "Clean shape, tests cover the right edges. Approving."
-- "Solid. One blocker inline — see the auth gap. Request changes."
+- "One small naming point inline, not a blocker. Approving."
+- "One edge-case gap noted inline, minor. Approving."
+- "One blocker inline — see the auth gap. Request changes."
+- "Approving." (nothing to flag)
-Bad (chatty, padded, marketing-adjacent):
+Bad (chatty, padded, or any praise on an approve):
- "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."
+- "Clean, good coverage. One naming point inline. Approving." — the leading praise is now cut; lead with the pointer instead.
+- "Solid fix. Approving." — a bare positive is still praise; drop it.
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.