From 1ed02105c0fa35049dd75f76ce63532ef1d64b2f Mon Sep 17 00:00:00 2001 From: Dan Mordechay Date: Thu, 23 Jul 2026 00:46:07 +0300 Subject: [PATCH] go-ldd v2.9.0: empathy test, decoder-ring refs, repo-idiom, when-in-Rome check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lessons from a repo owner's hard review of a large generated PR. The reviewer could merge the code but "could not reasonably ask another human to maintain the result". - R9 test 3 becomes the plain-English/empathy test: write for a fresh graduate whose first language may not be English; unexplained acronyms and jargon fail; self-standing — a comment you can only understand after reading the code has negative value. Comment-critic now reads each comment before the surrounding code, and notes a rename recommendation when the jargon lives in the symbol name. - Decoder-ring references (plan/decision/test-plan IDs, requirement tags, spec sections) join provenance in the anti-toolbox, with a worked example. - Restated repo idiom is a new floor failure mode; the critic greps the repo for the pattern before crediting such a WHY. - See-edge is budget-free only as its own trailing line. - When-in-Rome check in pre-commit-review + new "New Practice" finding category: new test mechanisms, deps, or conventions in a feature diff are adoption decisions for the repo owner — discussion or separate PR. - Two new maxims: "Empathy is a core engineering value" and "When in Rome, code as the Romans do". Co-Authored-By: Claude Fable 5 --- .../.claude-plugin/plugin.json | 2 +- go-linter-driven-development/CHANGELOG.md | 48 ++++++++++++++ .../agents/comment-critic.md | 36 ++++++++-- go-linter-driven-development/maxims.md | 27 ++++++++ .../rules/R9-repo-brain.md | 40 +++++++++--- .../skills/documentation/SKILL.md | 11 +++- .../skills/documentation/reference.md | 65 ++++++++++++++++--- .../skills/pre-commit-review/SKILL.md | 17 ++++- 8 files changed, 217 insertions(+), 29 deletions(-) diff --git a/go-linter-driven-development/.claude-plugin/plugin.json b/go-linter-driven-development/.claude-plugin/plugin.json index c39c0e7..7b1d903 100644 --- a/go-linter-driven-development/.claude-plugin/plugin.json +++ b/go-linter-driven-development/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "go-linter-driven-development", - "version": "2.8.0", + "version": "2.9.0", "description": "Rules-as-data linter-driven development workflow for Go: 12 single-source rule files, thin directional skills (design, TDD implementation, refactoring, testing, review, documentation), and a hunter/skeptic/critic review architecture with parallel single-rule reviewers, an over-abstraction skeptic, and a comment critic enforcing the Comment Value Toolbox", "author": { "name": "Dan Mordechay" diff --git a/go-linter-driven-development/CHANGELOG.md b/go-linter-driven-development/CHANGELOG.md index d6c12f4..fc17ef3 100644 --- a/go-linter-driven-development/CHANGELOG.md +++ b/go-linter-driven-development/CHANGELOG.md @@ -3,6 +3,54 @@ All notable changes to the `go-linter-driven-development` plugin are documented here. Format based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions follow [Semantic Versioning](https://semver.org/). +## [2.9.0] - 2026-07-23 + +Lessons from a repo owner's hard review of a large generated PR: the reviewer +could merge the code but "could not reasonably ask another human to maintain +the result". Two themes: comments that need the code (or a decoder ring) to be +understood, and a feature PR that silently adopted new engineering practices. + +### Added + +- **Empathy test — the plain-English test grows teeth** (R9 test 3, renamed + plain-English/empathy test). New persona anchor: write for a fresh graduate + whose first language may not be English. New failure modes: unexplained + acronyms and insider jargon ("DTO", "tristate") in comments AND in the symbol + names they document; and **self-standing** — a comment must be understandable + BEFORE reading the code ("if I need to read the code to understand the + comment, the comment adds negative value"); forward references to other + comments fail. The comment-critic now reads each comment before the + surrounding code so the self-standing check is built into its protocol. +- **Decoder-ring references join the provenance anti-toolbox** (R9 floor + + toolbox catalog + critic): plan/decision/test-plan IDs ("T-04-02", "D-07"), + requirement tags ("REQ-SVC-01"), spec section refs ("spec §4") fail even + when they resolve inside a repo doc — the fact goes in the comment as plain + prose, the doc through one See-edge, the ID stays in the doc. Worked ❌/✅ + example (`userResponse`) added to the catalog. +- **Restated repo idiom is a floor failure mode** (R9 + catalog + critic): a + comment justifying a convention the repo applies everywhere (pointer field = + "omitted vs explicit zero") is noise even though it is technically a WHY — + the convention lives once at rung 2. The critic greps the repo for the same + pattern before crediting such a rationale. +- **See-edge placement rule** (R9): the `See docs/.md` line is free + under the budget ONLY as its own trailing line; a doc reference braided into + the summary sentence is clutter, not an edge. +- **When-in-Rome check** (@pre-commit-review step 1 + new 🟠 New Practice + category + new maxim): anything the diff introduces that the host repo does + not already use — test mechanisms (golden files), dependencies, tools, config + conventions, repo-level file edits bundled into a feature diff — is flagged + as an adoption decision that belongs to the repo owner; the fix is a + discussion or a separate PR, never silent inclusion. Detection is + comparative: grep the repo outside the diff for prior use. +- **Two new maxims**: "Empathy is a core engineering value" (borrowed from a + reviewer's coding standards) under Clarity and knowledge; "When in Rome, + code as the Romans do" under Process and economics. +- **Name-jargon note in the critic**: renames are not the critic's to order, + but when the empathy test fails because the jargon lives in the symbol name + itself ("DTO"), the verdict block carries a rename recommendation for the + caller to route. (Added after a live run where the critic rewrote a comment + but left the jargon name unremarked.) + ## [2.8.0] - 2026-07-20 ### Added diff --git a/go-linter-driven-development/agents/comment-critic.md b/go-linter-driven-development/agents/comment-critic.md index b12261c..18b6594 100644 --- a/go-linter-driven-development/agents/comment-critic.md +++ b/go-linter-driven-development/agents/comment-critic.md @@ -33,13 +33,19 @@ comments. Directives (`//go:`, `//nolint`, `// Output:`) are not comments; skip them. **Critique protocol, per comment:** -1. Classify the symbol's tier (helper / contract / crossroads) from its role in +1. Read the comment BEFORE the surrounding code, and note whether you understood + it standing alone. This ordering is itself the self-standing half of test 3: + a comment you only understood after reading the code fails ("if I need to + read the code to understand the comment, the comment adds negative value"). +2. Classify the symbol's tier (helper / contract / crossroads) from its role in the code — Read the surrounding code, don't guess from the name. -2. Run the three tests from the payload, in order: toolbox-value (floor per line, - then ceiling for the whole comment against the tier), budget, plain English. -3. For any failure, decide the smallest verdict that fixes it: cut lines (TRIM), +3. Run the three tests from the payload, in order: toolbox-value (floor per line, + then ceiling for the whole comment against the tier), budget, plain English + + self-standing (the empathy test — judge it for a fresh graduate whose first + language may not be English; unexplained acronyms and insider jargon fail). +4. For any failure, decide the smallest verdict that fixes it: cut lines (TRIM), replace content (REWRITE), or remove entirely (DELETE). -4. Every TRIM/REWRITE ships the proposed replacement text, and the proposal names +5. Every TRIM/REWRITE ships the proposed replacement text, and the proposal names the toolbox item it delivers ("swap narrated implementation for the boundary contract this parsing constructor needs"). A bare "too long" is not a verdict. @@ -52,6 +58,26 @@ rationale: "silently resolving by precedence would pick a mode the caller didn't ask for"). An incident/ticket reference survives only when it IS the rationale for a constraint. +**Decoder-ring references are provenance in a different costume:** +plan/decision/test-plan IDs ("T-04-02", "D-07"), requirement tags +("REQ-SVC-01"), spec section refs ("spec §4"). They fail even when the token +resolves inside a repo doc — a reader without the decoder ring gets nothing. +REWRITE: the fact as plain prose, the doc via one trailing See-edge, the ID +gone. + +**Jargon in the symbol name:** your verdicts are about comments, and a rename +is not yours to order. But when the empathy test fails because the jargon +lives in the symbol name itself ("DTO", "mgr"), say so — append a +`note: symbol name carries the jargon — recommend rename (e.g. userDTO → +userResponse)` line to the verdict block so the caller can route it. + +**Repo idiom is not a WHY:** before crediting a rationale that justifies a +mechanical pattern (a pointer field for "omitted vs explicit zero", the +standard error-wrapping style), grep the repo for the same pattern. If it +appears across packages uncommented, this comment restates a repo-wide +convention — verdict DELETE; the convention's home is the coding-standards doc, +not a use site. + **Boundary with R3:** an in-body comment that names what the next block does is an extraction candidate, not a rewrite candidate — verdict `DELETE → route R3` (the fix is a function named after the comment, which is R3-storifying's diff --git a/go-linter-driven-development/maxims.md b/go-linter-driven-development/maxims.md index 1f05cb8..6aec575 100644 --- a/go-linter-driven-development/maxims.md +++ b/go-linter-driven-development/maxims.md @@ -190,6 +190,19 @@ imagined 30%. (R12's ceremony-copy inverse is one instance: don't "optimize" *or **Uncompiled** — no rule owns profile-first optimization discipline yet. +### When in Rome, code as the Romans do +— folk wisdom; sharpened by a repo owner's hard review of a large generated PR + +**Ask:** does this diff arrive in the host repo's existing style, or does it +import mine? A new test mechanism, dependency, framework, or convention — however +good — is an *adoption decision* that belongs to the repo owner, not a side +effect of a feature PR. Strong opinions travel as a discussion or a separate PR, +never as a bundled surprise. Reviewers forgive imperfect code in the house style +far more readily than perfect code in a foreign one. + +**Compiled into:** @pre-commit-review's when-in-Rome check (step 1) and its +🟠 New Practice finding category. + ## Clarity and knowledge ### Clear is better than clever @@ -200,6 +213,20 @@ future reader. **Compiled into:** `rules/R3-storifying.md` and the plugin's readability philosophy. +### Empathy is a core engineering value +— borrowed from a reviewer's coding standards ("coding in a way that facilitates +maintenance by peers who may be less skilled or experienced") + +**Ask:** could a fresh graduate whose first language may not be English read this +comment — before reading the code — and understand it? If you need to read the +code to understand the comment, the comment adds negative value. Codenames, +unexplained acronyms, and cross-reference webs serve the writer's bookkeeping, +not the reader. + +**Compiled into:** `rules/R9-repo-brain.md`'s plain-English/empathy test (test 3 +of the three-test standard: persona, acronym ban, self-standing requirement) and +the comment-critic's read-comment-first protocol. + ### Once and only once — Kent Beck (and DRY: "every piece of knowledge has a single, unambiguous, authoritative representation" — Hunt & Thomas) diff --git a/go-linter-driven-development/rules/R9-repo-brain.md b/go-linter-driven-development/rules/R9-repo-brain.md index 420a4cc..dedfcef 100644 --- a/go-linter-driven-development/rules/R9-repo-brain.md +++ b/go-linter-driven-development/rules/R9-repo-brain.md @@ -201,8 +201,17 @@ comment-critic agent enforces them adversarially after writing: - *Floor (delete test):* a prose line that delivers none of the toolbox values is trash — cut it. Named failure modes: restated identifiers, generic filler ("provides validation functionality"), narrated implementation, menu sections - filled without earning their place, and **provenance** — PR numbers, review - items, "the previous behavior" narration, "matching what did". + filled without earning their place, **restated repo idiom** — a comment + justifying a convention the repo already applies everywhere (a pointer field + meaning "omitted vs explicit zero", the standard error-wrapping style); the + convention is documented once at rung 2 (coding standards), never + re-explained at each use site — and **provenance and decoder-ring + references** — PR numbers, review items, plan/decision/test-plan IDs + ("T-04-02", "D-07"), requirement tags ("REQ-SVC-01"), spec section refs + ("spec §4"), "the previous behavior" narration, "matching what + did". A decoder-ring token fails even when it resolves inside a + repo doc: the reader gets the fact as plain prose and the doc through the + one See-edge — never through a code they must look up. The floor's lens is the 5-year reader test: a reader five years out cares how the product behaves NOW, never which PR or review round produced it. History is rewritten as present-tense rationale ("silently resolving by precedence @@ -215,16 +224,25 @@ comment-critic agent enforces them adversarially after writing: WHY is missing fails, even though each line individually "adds something". 2. **Budget test** — the comment fits its tier budget (accounting and tiers below). -3. **Plain-English test** — plain English only: everyday words, short sentences, - one idea per sentence. Not all readers are native English speakers; a comment - that needs a dictionary fails even when true and within budget. Failure modes: - fancy vocabulary where a common word exists ("utilize" → "use", "leverages" → - "uses"), stacked clauses, academic phrasing. +3. **Plain-English test (the empathy test)** — write for a fresh graduate whose + first language may not be English: everyday words, short sentences, one idea + per sentence. A comment that needs a dictionary fails even when true and + within budget. Failure modes: fancy vocabulary where a common word exists + ("utilize" → "use", "leverages" → "uses"), stacked clauses, academic phrasing, + and unexplained acronyms or insider jargon ("DTO", "tristate") — in the + comment AND in the symbol name it documents. + The test has a second half, **self-standing**: the comment must be + understandable BEFORE reading the code. If the reader must read the code — or + another comment ("see X's doc comment for why") — to understand this comment, + it has negative value. State the fact in place; forward references to other + comments fail. **Budget accounting** — prose lines count; these are free: - blank `//` separator lines -- the `See docs/.md` network-edge line +- the `See docs/.md` network-edge line — free ONLY as its own trailing + line; a doc reference woven into a prose sentence is not an edge, it is clutter + in that sentence's line count - short inline example lines, bounded at 2–4 lines — anything bigger belongs in an `Example_*` testable example @@ -261,8 +279,10 @@ rule's (Q4 below — they must carry why/context, not restate the identifier). ### Edge conventions - **Code → docs** (rung 1 → rung 2): a literal relative path in the comment — - `See docs/retry-policy.md`. Paths to docs are fine; docs move rarely and Q2 - verifies them mechanically. + `See docs/retry-policy.md` — always on its own trailing line, never braided + into the summary sentence (the first sentence stays clean: "Package accounts + registers the /accounts REST endpoints.", then the See-line). Paths to docs + are fine; docs move rarely and Q2 verifies them mechanically. - **Docs → code** (rung 2 → rungs 0–1): cite by **exported symbol** (`Policy`, `ParsePolicy`); a **package or directory path** (`retry/`) only when a location is genuinely needed; **file paths never, line numbers never** — they are the most diff --git a/go-linter-driven-development/skills/documentation/SKILL.md b/go-linter-driven-development/skills/documentation/SKILL.md index 8e1680a..0e12bae 100644 --- a/go-linter-driven-development/skills/documentation/SKILL.md +++ b/go-linter-driven-development/skills/documentation/SKILL.md @@ -45,9 +45,14 @@ entry (worked examples in reference.md, "Bug Fix Documentation"). doc that gets skipped. **Plain words over clever words**: everyday English, short sentences, one idea per -sentence. Not all readers are native English speakers — a doc that needs a -dictionary fails even when it is true (R9's plain-English test; applies to godocs -and feature docs alike). +sentence. Write for a fresh graduate whose first language may not be English — a +doc that needs a dictionary fails even when it is true (R9's plain-English/empathy +test; applies to godocs and feature docs alike). + +**Comments stand alone**: a reader must understand the comment BEFORE reading the +code. If reading the code is required to understand the comment, the comment adds +negative value — no decoder-ring IDs, no forward references to other comments +(R9's empathy test, second half). diff --git a/go-linter-driven-development/skills/documentation/reference.md b/go-linter-driven-development/skills/documentation/reference.md index 94740bd..8d8914a 100644 --- a/go-linter-driven-development/skills/documentation/reference.md +++ b/go-linter-driven-development/skills/documentation/reference.md @@ -93,7 +93,7 @@ the budget. // ✅ See docs/retry-policy.md for the incident and the cap math. ``` -### What never earns a line: provenance +### What never earns a line: provenance and decoder-ring references The anti-toolbox. PR numbers, review items, "the previous behavior" narration, "matching what did" — change history, not behavior. The 5-year @@ -103,17 +103,56 @@ present-tense rationale; keep an incident/ticket reference only when it IS the rationale for a constraint. ```go -// ❌ ...must be rejected, matching the D backend ("more than one TLS option -// passed"), REST v2 (which forwarded the conflict), and the CLI's own -// client-side mutual exclusion. Silently resolving by precedence — the -// previous behavior — picks a TLS mode the caller didn't ask for -// (PR #1088 review item 5a). +// ❌ ...must be rejected, matching the legacy backend ("more than one TLS +// option passed"), REST v2 (which forwarded the conflict), and the CLI's +// own client-side mutual exclusion. Silently resolving by precedence — +// the previous behavior — picks a TLS mode the caller didn't ask for +// (PR #481 review item 5a). // ✅ Naming more than one mutually-exclusive TLS option is rejected (422): // silently resolving by precedence would pick a mode the caller didn't // ask for. ``` +**Decoder-ring references** are the same failure in a different costume: +plan/decision/test-plan IDs ("T-04-02", "D-07"), requirement tags +("REQ-SVC-01"), spec section refs ("spec §4"). They fail even when the token +resolves inside a repo doc — a reader without the decoder ring gets nothing. +The fact goes in the comment as plain prose; the doc gets ONE trailing +See-edge; the ID stays in the doc. + +```go +// ❌ userResponse is the flat REST response shape for a user account +// (spec §4). It deliberately has NO field for the password hash — the +// response omission is structural (T-04-02), not a zero-value +// coincidence ... additive to the {uid} addressing scheme (D-06/D-07). + +// ✅ userResponse is the flat REST response shape for a user account. +// It has no field for the password hash or the API token, so a response +// can never leak them. +// See docs/accounts-api.md. +``` + +### What never earns a line: restated repo idiom + +A comment justifying a convention the repo already applies everywhere: a +pointer field meaning "omitted vs explicit zero", the standard error-wrapping +style, the usual table-driven test shape. Each use site inherits the +convention silently; the explanation lives once at rung 2 (coding standards). +If every site carried it, the real content would drown — and the one genuine +WHY nearby would read as more boilerplate. + +```go +// ❌ // Enabled is a pointer so an omitted field is distinguishable from an +// // explicit false. +// Enabled *bool `json:"enabled,omitempty"` + +// ✅ Enabled *bool `json:"enabled,omitempty"` +``` + +Test before crediting a WHY: grep the repo for the same pattern. If it appears +across packages with no comment, this comment restates an idiom — cut it. + --- ## Godoc Menus @@ -443,8 +482,15 @@ vets after the edit. - [ ] Every prose line delivers a Comment Value Toolbox item (floor), and the comment carries the highest-value items for its symbol's tier (ceiling) — R9's toolbox-value test -- [ ] Plain English throughout: everyday words, short sentences — no vocabulary a - non-native reader would need a dictionary for (R9's plain-English test) +- [ ] Plain English throughout: everyday words, short sentences, no unexplained + acronyms or insider jargon — written for a fresh graduate whose first + language may not be English (R9's plain-English/empathy test) +- [ ] Every comment is self-standing: understandable BEFORE reading the code, no + forward references to other comments (R9's empathy test, second half) +- [ ] No repo-idiom restating: a convention the repo applies everywhere is never + re-justified at a use site (documented once at rung 2) +- [ ] No decoder-ring references: no plan/decision/test-plan IDs, requirement + tags, or spec section refs — facts as prose, the doc via one See-edge - [ ] Exported symbols carry WHY — rationale, incident, constraint — never a restated identifier - [ ] Every doc comment fits its tier budget — helper 0–1 / contract 2–3 / @@ -454,7 +500,8 @@ vets after the edit. within the tier budget - [ ] Crossroads that deserve richer inline godoc got an expand recommendation in the report — never extra lines beyond budget -- [ ] `See docs/.md` edge present wherever a feature doc exists +- [ ] `See docs/.md` edge present wherever a feature doc exists — on its + own trailing line, never woven into the summary sentence - [ ] Testable examples: at least one `Example_*` per complex/core type; runnable; happy path only; `// Output:` comments included diff --git a/go-linter-driven-development/skills/pre-commit-review/SKILL.md b/go-linter-driven-development/skills/pre-commit-review/SKILL.md index f09d8c1..4e64c30 100644 --- a/go-linter-driven-development/skills/pre-commit-review/SKILL.md +++ b/go-linter-driven-development/skills/pre-commit-review/SKILL.md @@ -4,7 +4,7 @@ description: | ADVISORY pre-commit review that orchestrates parallel single-obsession rule hunters, an over-abstraction skeptic, and a comment critic against the diff. Spawns read-only agents (rule-hunter, overabstraction-skeptic, comment-critic); NEVER edits code. Invoked by @linter-driven-development (Phase 4), by @refactoring (after pattern application), or manually for standalone code review. - Categorizes findings as Bugs, Design Debt, Readability Debt, or Polish Opportunities. Does NOT block commits. + Categorizes findings as Bugs, New Practice (when in Rome), Design Debt, Readability Debt, or Polish Opportunities. Does NOT block commits. allowed-tools: - Read - Grep @@ -61,6 +61,16 @@ A rule with zero hits is skipped — no hunter spawned for it. Also in-context: a new `//nolint` directive or `.golangci.yaml` exclusion in the diff is itself a finding — the change must justify, with evidence, that the rule genuinely does not apply. + +Also in-context — the **when-in-Rome check**: a diff must arrive in the host repo's +existing style, not import a new one. Flag anything the diff introduces that the repo +does not already use: a new test mechanism (golden files, snapshot testing, a new +assertion library), a new dependency in `go.mod`, a new tool or config file, edits to +repo-level convention files (CLAUDE.md, coding standards, lint config) bundled into a +feature diff, or a directory layout unlike its siblings. Detection is comparative: +for each candidate, grep the repo *outside* the diff for prior use — zero prior use +is the finding. These are not style crimes; they are adoption decisions that belong +to the repo owner. The fix is a discussion or a separate PR, never silent inclusion. @@ -157,6 +167,11 @@ Category mapping: - 🐛 **Bugs** — will fail at runtime regardless of rule (nil returned as a value, cancellation swallowed by `context.Background()`, R10 goroutine leaks and unguarded concurrent writes): fix immediately. +- 🟠 **New Practice (when in Rome)** — the when-in-Rome check's findings: a + mechanism, dependency, framework, or convention the host repo does not already + use, introduced without discussion. Advisory like everything else, but flag it + loudly: reviewers reject these threads hardest, and the fix (owner buy-in or a + separate PR) is cheap before pushing and expensive after. - 🔴 **Design Debt** — R1, R2, R4, R6, R7, R8, R10's non-crash findings (production sleeps, fire-and-forget ownership, mutex placement), R11 (duplicated discriminators, boundary leaks), R12 (leaked mutable internals, unvalidated setters), and R5