diff options
| -rw-r--r-- | claude-rules/commits.md | 2 | ||||
| -rw-r--r-- | review-code/SKILL.md | 23 | ||||
| -rw-r--r-- | voice/SKILL.md | 6 | ||||
| -rw-r--r-- | voice/references/voice-profile.org | 15 |
4 files changed, 27 insertions, 19 deletions
diff --git a/claude-rules/commits.md b/claude-rules/commits.md index 4e23a75..c4eb2cd 100644 --- a/claude-rules/commits.md +++ b/claude-rules/commits.md @@ -386,7 +386,7 @@ Pick the shape first. Most reviews are Shape 1. 3. Run `/voice personal` on the file once. The skill walks its full pattern list across every block at the same time. The separators stay intact because they aren't prose. -4. Print the final draft inline in the terminal. Every block, exactly as it'll be posted, with its separator header. State that the skill ran (e.g. "/voice personal — full pattern walk across summary + 3 inline"). Surface any pattern #39 warnings. +4. Print the final draft inline in the terminal. Every block — the summary body AND the full prose of every inline comment — exactly as it'll be posted, with its separator header. Print the inline in full; never describe it in place of printing it ("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. State that the skill ran (e.g. "/voice personal — full pattern walk across summary + 3 inline"). Surface any pattern #39 warnings. 5. Ask: approve, request changes, or open in editor. Wait for an explicit answer. Do not open the file in `emacsclient` (or any editor) by default. - **Approve** → continue to step 6. 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. diff --git a/voice/SKILL.md b/voice/SKILL.md index 4690b97..19eb38b 100644 --- a/voice/SKILL.md +++ b/voice/SKILL.md @@ -44,9 +44,9 @@ Terse is a budget, not an adjective. Each publish-artifact type has a target sha |----------|--------| | Commit body | Skip entirely when the subject line carries the change. Otherwise short paragraphs: the constraint, bug, or tradeoff. No play-by-play. | | PR description | Problem / Fix / Why / Testing, each section tight. | -| PR review summary | One long sentence or a few short ones. Verdict closes it. Verdict formulas ("Approving.", "Requesting changes.") are valid sentences here. | +| PR review summary | Lead with the substantive pointer, verdict closes it. No praise, not even a bare positive (#40). Verdict formulas ("Approving.", "Requesting changes.") are valid sentences here. | | Inline pin (finding) | ~4 sentences in stems shape (#42): where the bug is, the fix, why it's better. | -| Praise comment | One sentence naming what's good. Nothing else (#40). | +| Praise comment (inline only) | One sentence naming what's good. Nothing else (#40). Never in the summary body. | | Follow-up approval after prior feedback was addressed | Exactly "Approved." | ## Your Task @@ -348,7 +348,7 @@ See `voice/references/voice-profile.org` §39 for problem, basis, examples, and ### 40. Praise vs Correction Asymmetry [personal] -**Rule.** Praise on a PR review is short and unjustified (the author knows why their good change is good). Correction always explains the why, gently and briefly, the way a mentor would. Never as a verdict from on high. **Verification narration is the same defect as justified praise:** "I traced X and it's safe because..." pads the compliment with the reviewer's homework. Tracing the code is the reviewer's job, not content for the comment — if verification found a problem, the problem gets the words; if it found nothing, it gets zero words. +**Rule.** Praise on a PR review is short and unjustified (the author knows why their good change is good). Correction always explains the why, gently and briefly, the way a mentor would. Never as a verdict from on high. **Verification narration is the same defect as justified praise:** "I traced X and it's safe because..." pads the compliment with the reviewer's homework. Tracing the code is the reviewer's job, not content for the comment — if verification found a problem, the problem gets the words; if it found nothing, it gets zero words. **An approve summary carries no praise at all** — not even a bare positive ("Clean.", "Solid fix."). Lead the summary with the substantive pointer (the design note pinned inline) and close with the verdict: "One design note inline, not a blocker. Approving." An approve with nothing to flag is just "Approving." Short unjustified praise survives only as an inline pin on the line it refers to, never in the summary body. See `voice/references/voice-profile.org` §40 for problem, basis, examples, and history. diff --git a/voice/references/voice-profile.org b/voice/references/voice-profile.org index 088f0eb..765d513 100644 --- a/voice/references/voice-profile.org +++ b/voice/references/voice-profile.org @@ -1308,9 +1308,9 @@ Local absolute paths (=/home/<user>/...=, =/Users/<user>/...=), private repo nam Personal mode only. General and prose skip because the rule assumes a PR review context. *** Rule -Praise on a PR review is short and unjustified (the author knows why their good change is good). Correction always explains the why, gently and briefly, the way a mentor would, never as a verdict from on high. Keep it brief either way. +Praise on a PR review is short and unjustified (the author knows why their good change is good), and it survives only as an inline pin on the line it refers to. Correction always explains the why, gently and briefly, the way a mentor would, never as a verdict from on high. Keep it brief either way. -On an approve summary: praise plus verdict, nothing else. Cut any clause that describes or justifies the change. "Clean fix on the stacking bug, the tri-state is the right level to solve it at, and the tests cover the edges. Approving." becomes "Clean fix on the stacking bug. Approving." If a clause references what the code does or why it works, delete it. +On an approve summary: no praise at all, not even a bare positive ("Clean.", "Solid fix."). Lead with the substantive pointer — the design note pinned inline — and close with the verdict; an approve with nothing to flag is just "Approving." "Clean fix on the stacking bug, the tri-state is the right level to solve it at, and the tests cover the edges. Approving." becomes "One design note inline, not a blocker. Approving." (or just "Approving." with nothing to flag). Cut any clause that describes, justifies, or compliments the change — if a clause references what the code does, why it works, or how good it is, delete it. On a finding or change-request: always give the why, gently and briefly. Not "Move this to a helper." but "I'd pull this into one helper — three copies of the same rule means the next change has to touch all three, and missing one brings the bug back." @@ -1329,9 +1329,11 @@ Nice clean migration, the provider mocks and the Normal/Boundary/Error cases are *** After #+begin_example -Clean migration. Approving. One note inline: I'd rename `x` to `provider` — it reads as a generic placeholder and the next person won't know it's the resolved provider without tracing it. +One naming note inline, not a blocker. Approving. #+end_example +The rename rationale (`x` reads as a generic placeholder; the next person won't know it's the resolved provider without tracing it) lives in the inline pin, not the summary — the summary points, the pin teaches. + *** Before (verification narration) #+begin_example All three fixes look right. I traced useMapActions and the unmount cleanup is safe because the hook returns a memoized object, and the provider wraps the whole app so neither call site lands on the no-op path. @@ -1339,16 +1341,19 @@ All three fixes look right. I traced useMapActions and the unmount cleanup is sa *** After #+begin_example -All three fixes are clean and well-aimed. +Approving. #+end_example +Nothing to flag, so the summary is the bare verdict. The old "All three fixes are clean and well-aimed" is itself praise, and praise is now cut from the approve summary entirely. + *** Detection -In a PR review summary or comment: a praise clause that explains why the good thing is good, a praise clause followed by the verification work that supports it, or a finding or change-request that states what to fix without saying why. +In a PR review summary or comment: any praise on an approve summary (including a bare positive), a praise clause that explains why the good thing is good, a praise clause followed by the verification work that supports it, or a finding or change-request that states what to fix without saying why. *** History - Original SKILL.md entry: praise-versus-correction asymmetry for PR review. - 2026-05-29: migrated to this file as the canonical home per the pairing rule. - 2026-06-10: verification-narration variant added after the third recurrence — a review draft praised a fix and then narrated the verification supporting the praise (the #236 draft). Added to the SKILL.md rule line and the high-recurrence attestation set. Craig's call, from the work-project session. +- 2026-07-11: bare-positive carve-out removed. An approve summary now carries no praise at all, not even "Clean." / "Solid fix." — lead with the substantive pointer, close with the verdict. Craig's ruling from a DeepSat review session (approved "One design note inline, not a blocker. Approving."). Same change applied to review-code's Posted Summary Voice and commits.md Shape 1. ** §41 No Emphasis Formatting |