diff options
| -rw-r--r-- | review-code/SKILL.md | 18 | ||||
| -rw-r--r-- | voice/SKILL.md | 26 |
2 files changed, 33 insertions, 11 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 diff --git a/voice/SKILL.md b/voice/SKILL.md index 9de3d71..af10275 100644 --- a/voice/SKILL.md +++ b/voice/SKILL.md @@ -1,7 +1,7 @@ --- name: voice description: | - Multi-pass prose editor with two modes. General mode (default) edits arbitrary writing — research notes, essays, emails, README prose — by walking 31 patterns: Wikipedia's Signs of AI Writing patterns plus universal good-writing rules (long-word → short-word, active-over-passive, comma splices, cliché flag, jargon-fragment-in-prose rewrite, corporate-speak nominalizations). Personal mode adds 8 more patterns for commits, PR titles + bodies, and PR review comments (first-person rewrite, semicolons → periods/commas, contractions, sentence-split on conjunctions, felt-experience narration cut, sentence-fragment-in-prose rewrite, terse-cut for rhetorical padding, public-artifact scope flag). Total 39 patterns; one editorial review covers all relevant ones for the chosen mode. Replaces the standalone humanizer skill. Use when editing prose. Do NOT use for code, structured data, or plain bullet lists where fragments are valid. + Multi-pass prose editor with two modes. General mode (default) edits arbitrary writing — research notes, essays, emails, README prose — by walking 31 patterns: Wikipedia's Signs of AI Writing patterns plus universal good-writing rules (long-word → short-word, active-over-passive, comma splices, cliché flag, jargon-fragment-in-prose rewrite, corporate-speak nominalizations). Personal mode adds 9 more patterns for commits, PR titles + bodies, and PR review comments (first-person rewrite, semicolons → periods/commas, contractions, sentence-split on conjunctions, felt-experience narration cut, sentence-fragment-in-prose rewrite, terse-cut for rhetorical padding, public-artifact scope flag, praise/correction asymmetry). Total 40 patterns; one editorial review covers all relevant ones for the chosen mode. Replaces the standalone humanizer skill. Use when editing prose. Do NOT use for code, structured data, or plain bullet lists where fragments are valid. allowed-tools: - Read - Write @@ -20,7 +20,7 @@ You are a writing editor that walks a numbered pattern list against a piece of t Two modes determine which patterns to walk. - **General** (default) — apply patterns **#1-31**. Use for any writing not bound for commits, PRs, or PR comments: research notes, philosophy or history essays, emails, README prose, journal entries, anything else. Output is well-edited human-sounding prose, but does not impose first-person voice, contraction enforcement, or other personal-style choices that conflict with academic, literary, or formal registers. -- **Personal** — apply all **#1-39**. Use only for commits, PR titles + bodies, and PR review comments. Patterns marked **(personal only)** are skipped in general mode because they conflict with non-publish registers (third-person is valid in academic prose, semicolons are conventional in long-form writing, fragments are valid stylistic devices in literary prose, felt-experience is the content of personal essays, and so on). +- **Personal** — apply all **#1-40**. Use only for commits, PR titles + bodies, and PR review comments. Patterns marked **(personal only)** are skipped in general mode because they conflict with non-publish registers (third-person is valid in academic prose, semicolons are conventional in long-form writing, fragments are valid stylistic devices in literary prose, felt-experience is the content of personal essays, and so on). If invoked without a mode argument, default to general. Personal-context callers (`commits.md` publish flow, `respond-to-cj-comments.md`) invoke this skill explicitly with `/voice personal`. @@ -541,10 +541,26 @@ WARN: line 12: "/home/cjennings/code/rulesets" — local absolute path in commit WARN: line 18: "claude-rules/commits.md" — personal-tooling reference; state the underlying reason instead ``` +### 40. Praise vs Correction Asymmetry (personal only) + +**Detection:** In a PR review summary or comment — a praise clause that explains *why* the good thing is good; or a finding / change-request that states what to fix without saying why. + +**Problem:** Praise and correction call for opposite treatment. The author already knows why their good change is good, so justifying praise reads as flattery — give the praise and stop. Correction is the reverse: behavior only changes when the reason lands, so a finding, change-request, or inline coaching note must always explain the why. And the why is delivered gently, the way a kind coach or 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 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." + +**Before:** +> Nice clean migration, the provider mocks and the Normal/Boundary/Error cases are all covered which is exactly what I'd want here. Approving. Also rename `x`. + +**After:** +> 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. + ## Process 1. Read the input text carefully. Confirm the mode (general or personal) — invocation argument or context. -2. Walk patterns 1-31 in general mode; walk all 39 patterns in personal mode. +2. Walk patterns 1-31 in general mode; walk all 40 patterns in personal mode. 3. For each pattern, scan the text. If a match is found, rewrite it according to the pattern's rule. Pattern #39 emits warnings without rewriting. 4. After walking all patterns, ensure the revised text: - Sounds natural when read aloud @@ -618,7 +634,7 @@ Provide: > The Statistical Institute of Catalonia was established in 1989 to collect and publish regional statistics independently from Spain's national statistics office. It was part of a wider movement to decentralize administrative functions across Spain. Regional reporting got more accurate after the change. **Summary of changes:** -- Patterns that fired in general mode: #1 (significance inflation: "pivotal moment", "evolution of"), #4 (promotional: "vital component"), #3 (-ing analysis: "showcasing how... can foster"), #8 (copula avoidance: "serves as"), #26 (long-word: removed Latinate constructions). General mode skipped patterns #32-39 (personal only). +- Patterns that fired in general mode: #1 (significance inflation: "pivotal moment", "evolution of"), #4 (promotional: "vital component"), #3 (-ing analysis: "showcasing how... can foster"), #8 (copula avoidance: "serves as"), #26 (long-word: removed Latinate constructions). General mode skipped patterns #32-40 (personal only). ## Reference @@ -628,7 +644,7 @@ This skill draws from: - Orwell, *Politics and the English Language* — patterns #26 (short over long), #27 (active over passive), #29 (cliché). - Plain English Campaign — pattern #26 (Plain English wordlist). - Garner, *Modern English Usage* — pattern #26 (word-pair preferences). -- Personal voice rules from `claude-rules/commits.md` (Voice and Focus section) — patterns #32-39. +- Personal voice rules from `claude-rules/commits.md` (Voice and Focus section) — patterns #32-40. Key insight (Wikipedia, paraphrased): LLMs use statistical algorithms to guess what should come next. The result tends toward the most statistically likely text that applies to the widest variety of cases. Patterns #1-25 detect that signature. |
