diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index a528213..d4c7888 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -7,7 +7,7 @@ { "name": "go-linter-driven-development", "source": "./go-linter-driven-development", - "description": "Rules-as-data linter-driven development workflow for Go: 10 single-source rule files, thin directional skills (design, TDD implementation, refactoring, testing, review, documentation), and a hunter/skeptic review architecture with parallel single-rule reviewers and an over-abstraction skeptic" + "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 review architecture with parallel single-rule reviewers and an over-abstraction skeptic" }, { "name": "ts-react-linter-driven-development", diff --git a/README.md b/README.md index 550e8db..87d91a5 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,8 @@ A [Claude Code](https://claude.ai/code) plugin marketplace for **linter-driven d | | Plugin | Version | For | |---|--------|---------|-----| -| 🐹 | [`go-linter-driven-development`](go-linter-driven-development/README.md) | 2.0.0 | Go | -| βš›οΈ | [`ts-react-linter-driven-development`](ts-react-linter-driven-development/README.md) | 1.0.0 | TypeScript + React | +| 🐹 | [`go-linter-driven-development`](go-linter-driven-development/README.md) | 2.6.0 | Go | +| βš›οΈ | [`ts-react-linter-driven-development`](ts-react-linter-driven-development/README.md) | 1.2.0 | TypeScript + React | Plus the standalone rule documents the plugins grew out of: @@ -18,10 +18,10 @@ Plus the standalone rule documents the plugins grew out of: The organising idea: **the rule is the unit, not the phase.** Each design principle lives exactly once, as data: -- **`rules/` R1–R10** β€” single source of truth: primitive obsession, self-validating types, storifying, helper placement, vertical slices, test-only interfaces, test placement, no globals, repo-brain documentation, concurrency safety. +- **`rules/` R1–R12** β€” single source of truth: primitive obsession, self-validating types, storifying, helper placement, vertical slices, test-only interfaces, test placement, no globals, repo-brain documentation, concurrency safety, conditional dispatch (Anti-IF), mutation discipline (Fowler's Mutable Data). - **`skills/`** β€” six thin directional views that sequence and route into the rules (orchestrator, design, testing, refactoring, review, documentation). - **`agents/`** β€” payload-fed isolated workers: parallel single-obsession `rule-hunter`s, an `overabstraction-skeptic` that tries to kill proposed extractions, and a `lint-fixer` that keeps the lint loop out of your conversation. -- **`commands/`** β€” `/go-ldd-autopilot`, `/go-ldd-quickfix`, `/go-ldd-analyze`, `/go-ldd-review`, `/go-ldd-status`, `/wire-repo-brain`. +- **`commands/`** β€” `/go-ldd-autopilot`, `/go-ldd-quickfix`, `/go-ldd-prepare`, `/go-ldd-analyze`, `/go-ldd-review`, `/go-ldd-status`, `/wire-repo-brain`. Full architecture, workflow, and usage: [plugin README](go-linter-driven-development/README.md) Β· what changed between versions: [CHANGELOG](go-linter-driven-development/CHANGELOG.md). diff --git a/go-linter-driven-development/.claude-plugin/plugin.json b/go-linter-driven-development/.claude-plugin/plugin.json index 03979df..c2183bc 100644 --- a/go-linter-driven-development/.claude-plugin/plugin.json +++ b/go-linter-driven-development/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "go-linter-driven-development", - "version": "2.1.0", - "description": "Rules-as-data linter-driven development workflow for Go: 10 single-source rule files, thin directional skills (design, TDD implementation, refactoring, testing, review, documentation), and a hunter/skeptic review architecture with parallel single-rule reviewers and an over-abstraction skeptic", + "version": "2.6.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 review architecture with parallel single-rule reviewers and an over-abstraction skeptic", "author": { "name": "Dan Mordechay" }, diff --git a/go-linter-driven-development/CHANGELOG.md b/go-linter-driven-development/CHANGELOG.md index 937397d..024e163 100644 --- a/go-linter-driven-development/CHANGELOG.md +++ b/go-linter-driven-development/CHANGELOG.md @@ -3,6 +3,164 @@ 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.6.0] - 2026-07-10 + +### Added + +- **Finding clusters β€” design-first routing for related review findings.** Hunters + are single-obsession and blind to each other, so when findings from β‰₯2 different + rules converge on one anchor (the same type, field/discriminator, or function), + that independent convergence is evidence of a missing domain concept β€” a + juiciness scorecard that filled itself in. Fixing such findings member-by-member + produces partial patches that undo each other (R1 names an enum that R11's move + then replaces; R2 places validation that R11's move relocates). + - `@pre-commit-review` step 4 gains a **cluster pass**: convergent findings are + reported as first-class πŸ”— CLUSTER entries with a root-cause hypothesis and a + design-first routing note; members stay in their categories, tagged. Clustering + is reporting β€” the skill still never edits. + - The orchestrator's Phase 4 gains **cluster routing**: clusters go to + `@code-designing` in a new **cluster-scoped mode** (skips the architecture scan + and user-OK gate β€” acceptance was inherited when the findings were accepted; + designs only the one concept the cluster names), then `@refactoring` implements + the mini plan; member findings resolve as consequences of one design. + Singletons route directly to `@refactoring`, unchanged. + - `/go-ldd-quickfix` Phase 4 follows the same routing. + +## [2.5.0] - 2026-07-10 + +### Added + +- **`maxims.md` β€” the uncompiled layer above the rules.** Rules are compiled + judgment (detection command + violation criterion + fix pattern); maxims are the + named questions that *generate* such rules in situations no rule anticipated: + "Tell, don't ask", the Law of Demeter, "Make illegal states unrepresentable", + "Parse, don't validate", "Duplication is far cheaper than the wrong abstraction", + "YAGNI", "Three strikes", "A little copying is better than a little dependency", + "The bigger the interface, the weaker the abstraction", "Make the zero value + useful" (held in explicit tension with R2), "Make the change easy…", "If a test + is hard to write, the design is wrong", "If it hurts, do it more often", + "Premature optimization…", "Clear is better than clever", "Once and only once", + "Depend in the direction of stability". Each entry: the quote, attribution, the + question it makes you ask, and the rules that compile it (or *uncompiled* status). +- **The contract: maxims propose, evidence disposes.** Maxims are wired into the + three judgment points and banned from the evidence path: + - @code-designing gains `` β€” the plan is questioned with the + maxims before types are committed (design has no diff to grep; questions are + the only tool there) + - @refactoring's escalation now frames *why code resists* in maxim vocabulary + - the `overabstraction-skeptic` cites Metz/Pike/YAGNI by name in verdicts β€” a + named principle is an argument, "feels unnecessary" is not + - the `rule-hunter` evidence protocol explicitly forbids maxim-justified findings +- **Graduation path**: a maxim that keeps generating findings no rule can express + gets compiled into an R-file β€” R11 and R4's feature-envy question are graduates + of exactly this path. +- **The Message Chains ↔ Middle Man pair** (Fowler's opposing smells), compiled to + its post-Tell-Don't-Ask residue as two R4 fix-pattern bullets: a chain is a + placement signal (move the behavior; only boundary egress keeps the chain, inside + the adapter), and a pure-forward method is R1's ceremony verdict applied per + method (delete forwards that own no rule; domain-type embedding manufactures the + smell in one line). No R13 β€” after the behavior moves, nothing detectable + remains, so the pair is guidance, not a rule. +- **House maxim: "Every indirection must earn its keep"** β€” the generalized + juiciness test, named as the plugin's own synthesis: one principle at six + granularities (type/interface/method/dispatch/guard/copy), enforcement agent the + `overabstraction-skeptic`; each rule's inverse trap is its retrospective form. + +## [2.4.0] - 2026-07-10 + +### Added + +- **Phase 1.5 PREPARE β€” autonomous preparatory refactoring** (Fowler: "make the + change easy, then make the easy change"). After the user approves the design plan, + the orchestrator surveys the plan's touch points with the existing rule detection + greps and reshapes what the change is about to hit β€” before the first RED, in its + own commit(s). Decisions are made by four mechanical gates, never by stopping to + ask: **MULTIPLY** (only violations the plan would multiply qualify), **SAFE** + (characterization tests first on uncovered paths), **BOUNDED** (S/M efforts + proceed, L defers to the Phase 4 report), **SKEPTICIZED** (any extraction is + judged by the over-abstraction skeptic, scored against the plan in hand). + Autopilot never pauses; the PREPARATION LOG is a record, not a question. +- **RED-friction escape hatch** (Phase 2): a RED test that resists β€” fixture + surgery, global mutation, driving layers to reach a seam β€” is a prep signal the + survey missed; suspend the cycle, run the same gates, land the prep commit, + re-enter RED. +- **`@refactoring` ``**: same routing table and moves, different + trigger (the plan, not the linter β€” targets are usually lint-green) and different + stopping criterion: not "linter green" but "the feature now lands add-only"; + full suite green after every move; prep commits segregated from feature commits. +- **`/go-ldd-prepare [files]`** β€” standalone entry: describe the impending + change, get the survey + gates + reshaping without the full five-phase workflow. + +## [2.3.0] - 2026-07-10 + +The Fowler wave β€” adopting the *Refactoring* (2nd ed.) ideas the rule set didn't +already embody. (Much of the catalog was already here under other names: Extract +Function β†’ R3, Replace Primitive with Object β†’ R1, Repeated Switches β†’ R11, +Speculative Generality β†’ the over-abstraction skeptic, the Two Hats β†’ the +REDβ†’GREENβ†’REFACTOR loop.) + +### Added + +- **`rules/R12-mutation-discipline.md`** β€” Fowler's Mutable Data smell family with + Go aliasing teeth: a validated value changes state only through methods that own + its invariants. In Go, `return g.perms` returns a mutable alias into the + "validated" state β€” R2's validate-once guarantee is void the moment an internal + slice escapes. R12 owns: + - Copy on the Way In β€” constructors clone slice/map arguments (or build fresh) + - Copy on the Way Out / Encapsulate Collection β€” queries return clones or `iter.Seq` iterators, never internal references + - Separate Query from Modifier β€” split hybrids; pure naming cases stay with R3's Honest Rename + - Remove Setting Method β€” no unvalidated mutation paths around a validating constructor (construction itself stays R2's) + - Split Variable β€” one assignment per meaning + - the inverse trap: ceremony copies of data that never escapes, mirroring R1's over-abstraction symmetry +- **Split Phase move in R3** (Fowler's opening example): a function interleaving + parsing with computation splits into phase 1 producing an intermediate domain + structure and phase 2 consuming it β€” distinct from Extract Function because it + introduces a data structure *between* the steps; collapses into R2's `ParseX` + when phase 1 validates. +- **Data Clumps question in R1** (Introduce Parameter Object / Preserve Whole + Object): the same parameter group in β‰₯2 signatures is a type asking to exist β€” + the scorecard already rewarded grouping; now a falsifying question hunts it. +- **Feature Envy in R4** (Move Function): a new fix pattern (Move Method to the + Envied Type) and falsifying question β€” a function reading a foreign type's data + more than its own moves onto that type, then re-places via the ladder. +- Wiring: pre-commit-review hunts R12 (πŸ”΄ Design Debt), code-designing dispatches + R12 at design time (closed mutation surface in the checklist), refactoring's + pattern index owns the five R12 moves. R12 has no owning linter (like R9) β€” no + routing-table row. + + + +## [2.2.0] - 2026-07-10 + +### Added + +- **`rules/R11-conditional-dispatch.md`** β€” the Anti-IF rule, adapting the Anti-IF + movement's core insight (Cirillo, 2007) to the rules-as-data architecture: a + conditional that asks what a value *is* may exist once; the second copy of a + kind/type discriminator is a missing polymorphic type. R11 owns: + - Replace Duplicated Switch with Interface Dispatch β€” variants become leaf types, the decision moves to a `ParseX` boundary constructor (R2's behavioral twin: *decide* once at the edge) + - Replace If-Chain with Strategy Map β€” single-behavior variance dispatches through a map, comma-ok at the boundary only + - Introduce Null Object and Split Flag Argument + - the sanctioned form: Keep the Single Exhaustive Switch β€” one site over a closed enum stays, named per R1 and proven complete by the `exhaustive` linter + - the inverse trap: dispatch abstractions that delete no duplication are ceremony β€” one switching site with trivial variance keeps its switch (mirroring R1's over-abstraction symmetry and R6's earned-interface test) +- **`examples/anti-if-dispatch.md`** β€” case law: a three-site channel switch (already + drifted) collapsed to interface dispatch, the strategy-map variant, and the worked + rejection where the skeptic kills the extraction and the switch goes exhaustive + instead. Pasted to the skeptic alongside R11 dispatch proposals. +- **`examples/switch-to-polymorphism.md`** β€” second R11 case file (real production + code), the type-switch sibling of anti-if-dispatch: an already-polymorphic value + un-dispatched by a field-unpacking type switch becomes a fill-style interface + method. Covers the tempting wrong fix (extract-per-case as ceiling, not cure), + fill-don't-construct ownership, the earned + sealed interface (R6), and the + dependency-direction rejection β€” orthogonal to the juiciness rejection β€” where + the consumer owns the wire format and the switch legitimately stays as pure + dispatch. +- Wiring: pre-commit-review hunts R11 (πŸ”΄ Design Debt), its dispatch proposals face + the over-abstraction skeptic with the new case file as payload, code-designing + dispatches R11 at design time (one dispatch owner per variant family in the + checklist), refactoring + lint-fixer route `exhaustive` failures and + discriminator-shaped `dupl` hits to it. + ## [2.1.0] - 2026-07-07 ### Added @@ -58,6 +216,11 @@ Initial release as a Claude Code plugin: five-phase linter-driven workflow (desi Notable unversioned improvements between 1.0.0 and 2.0.0: auto-pilot mode and review agent commands, evidence-based review with test-only interface detection, self-validation ownership rule, improved lint-failure flow, and making the package-size hook opt-in. +[2.6.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.6.0 +[2.5.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.5.0 +[2.4.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.4.0 +[2.3.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.3.0 +[2.2.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.2.0 [2.1.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.1.0 [2.0.0]: https://github.com/buzzdan/ai-coding-rules/releases/tag/go-ldd-v2.0.0 [1.0.0]: https://github.com/buzzdan/ai-coding-rules/commit/746ae7d diff --git a/go-linter-driven-development/README.md b/go-linter-driven-development/README.md index 3fb8947..3df8656 100644 --- a/go-linter-driven-development/README.md +++ b/go-linter-driven-development/README.md @@ -16,18 +16,21 @@ The organising idea of v2: **the rule is the unit, not the phase.** Each design ``` go-linter-driven-development/ -β”œβ”€β”€ rules/ R1-primitive-obsession … R10-concurrency-safety (single source of truth) -β”œβ”€β”€ examples/ storify-leaf-type Β· overabstraction-cidr Β· dependency-rejection (case law) +β”œβ”€β”€ maxims.md the uncompiled layer β€” named design questions above the rules +β”œβ”€β”€ rules/ R1-primitive-obsession … R12-mutation-discipline (single source of truth) +β”œβ”€β”€ examples/ storify-leaf-type Β· overabstraction-cidr Β· dependency-rejection Β· +β”‚ anti-if-dispatch Β· switch-to-polymorphism (case law) β”œβ”€β”€ skills/ linter-driven-development Β· code-designing Β· refactoring Β· β”‚ pre-commit-review Β· testing Β· documentation (thin directional views) β”œβ”€β”€ agents/ rule-hunter Β· overabstraction-skeptic Β· lint-fixer (isolated workers) -β”œβ”€β”€ commands/ go-ldd-analyze Β· autopilot Β· quickfix Β· review Β· status Β· wire-repo-brain +β”œβ”€β”€ commands/ go-ldd-analyze Β· autopilot Β· quickfix Β· prepare Β· review Β· status Β· wire-repo-brain └── hooks/ package-size gate ``` -**Four layers, one fact per fact:** +**Five layers, one fact per fact:** -- **[`rules/`](rules/)** β€” R1–R10, each a self-contained hunter payload. A rule file states its Principle, Why, a real-world canonical before/after, Design guidance (forward), a Fix pattern (backward), and Falsifying questions (each phrased to *disprove* compliance, with a grep/count detection command). A rule's content is normative in its file and nowhere else β€” everything else points at it. +- **[`maxims.md`](maxims.md)** β€” the layer above the rules: named design maxims ("Tell, don't ask", "Duplication is far cheaper than the wrong abstraction", "Make the change easy…") as *questions*, each pointing at the rules that compile it. Maxims live only at the judgment points β€” design interrogation (@code-designing), escalation vocabulary (@refactoring), the skeptic's doctrine β€” and are banned from hunters: **maxims propose, evidence disposes.** A maxim that keeps convicting graduates into a rule. +- **[`rules/`](rules/)** β€” R1–R12, each a self-contained hunter payload. A rule file states its Principle, Why, a real-world canonical before/after, Design guidance (forward), a Fix pattern (backward), and Falsifying questions (each phrased to *disprove* compliance, with a grep/count detection command). A rule's content is normative in its file and nowhere else β€” everything else points at it. - **[`examples/`](examples/)** β€” deep worked case studies (full before/after code + the reasoning). Rules cite them by relative path instead of inlining long studies. - **[`skills/`](skills/)** β€” thin directional views (~100–150 lines) that *sequence* and *route* into the rules. They never restate rule content. - **[`agents/`](agents/)** β€” read-only or mechanical workers spawned in isolated contexts. **Agents get knowledge as spawn-time payload β€” the relevant rule file's content is pasted into the prompt. Agents do NOT invoke skills.** @@ -38,8 +41,11 @@ The [`@linter-driven-development`](skills/linter-driven-development/SKILL.md) sk ``` 1 DESIGN @code-designing β†’ DESIGN PLAN β†’ user OK +1.5 PREPARE preparatory refactoring (Fowler): survey the plan's touch points, + four autonomous gates decide, @refactoring reshapes β†’ prep commit(s), no user stop 2 IMPLEMENT per behavior: β”Œβ”€> RED one failing test, lowest rung on the composition ladder (@testing) + β”‚ test resists? β†’ late prep signal β†’ same gates β†’ prep commit β†’ re-enter β”‚ GREEN minimum code to pass β€” no design work β”‚ REFACTOR package-scoped lint + rule greps; any hit β†’ @refactoring └── next behavior until all done @@ -49,7 +55,7 @@ The [`@linter-driven-development`](skills/linter-driven-development/SKILL.md) sk 5 SHIP @documentation β†’ commit summary β†’ user commits ``` -Design happens once, up front (Phase 1); the RED test's shape carries that design into GREEN. The cheap per-cycle greps in Phase 2's REFACTOR are the mid-implementation net; the Phase 4 hunter/skeptic pass is the verification net on finished work. +Design happens once, up front (Phase 1); the RED test's shape carries that design into GREEN. PREPARE makes the change easy before making the easy change β€” reshaping only what the plan touches and only violations the plan would multiply, gated autonomously (the over-abstraction skeptic judges any extraction) so autopilot never stops to ask. The cheap per-cycle greps in Phase 2's REFACTOR are the mid-implementation net; the Phase 4 hunter/skeptic pass is the verification net on finished work. ### The Hunter / Skeptic Review Model @@ -58,7 +64,7 @@ Phase 4 ([`@pre-commit-review`](skills/pre-commit-review/SKILL.md)) is pure orch 1. **Grep pre-filter** (in-context, cheap): run each rule's detection commands against the diff. A rule with zero hits gets no hunter. 2. **Parallel hunters**: for every rule with hits, spawn one [`rule-hunter`](agents/rule-hunter.md) β€” single obsession, single rule file pasted in full as its entire rulebook. Each returns evidence-backed findings (`rule | file:line | falsifying-question answers | fix pattern | effort`). 3. **Skeptic pass**: every "create a type/package" proposal goes to one [`overabstraction-skeptic`](agents/overabstraction-skeptic.md), which tries to *kill* each extraction using R1's juiciness scorecard and the CIDR case file. A refuted proposal ships only its cheaper alternative (better naming, private fields + accessors). -4. **Merged report**: surviving findings categorized as πŸ› Bugs / πŸ”΄ Design Debt / 🟑 Readability Debt / 🟒 Polish. All advisory β€” the caller decides what to fix. +4. **Merged report**: surviving findings categorized as πŸ› Bugs / πŸ”΄ Design Debt / 🟑 Readability Debt / 🟒 Polish. All advisory β€” the caller decides what to fix. Findings from *different* rules converging on one anchor (the same type, field, or function) are additionally reported as a πŸ”— **CLUSTER** β€” each hunter is blind to the others, so independent convergence is evidence of a missing domain concept, and the cluster routes design-first (`@code-designing` scoped to the concept, then `@refactoring` implements) instead of being fixed member-by-member. Isolated contexts matter: the `lint-fixer` loop's token noise stays out of your conversation, and each hunter's fresh context is exactly what makes its findings trustworthy on finished work. @@ -78,6 +84,8 @@ Isolated contexts matter: the `lint-fixer` loop's token noise stays out of your | R8 | [`rules/R8-no-globals.md`](rules/R8-no-globals.md) | No package-level state; no `context.Background()` in library code | | R9 | [`rules/R9-repo-brain.md`](rules/R9-repo-brain.md) | Documentation network: fact at its lowest rung, reachable from the root, edges both directions; index wired into CLAUDE.md | | R10 | [`rules/R10-concurrency-safety.md`](rules/R10-concurrency-safety.md) | Goroutines with owners and exit paths; shared state guarded where it lives; no production sleeps | +| R11 | [`rules/R11-conditional-dispatch.md`](rules/R11-conditional-dispatch.md) | One dispatch owner per kind/variant family (Anti-IF): duplicated kind-switches become interface/map dispatch chosen once at the boundary; a single switch stays and goes exhaustive | +| R12 | [`rules/R12-mutation-discipline.md`](rules/R12-mutation-discipline.md) | Mutation only through invariant-owning methods: constructors copy collections in, queries copy (or iterate) out, no query/modifier hybrids, no setters around validating constructors | **Examples β†’ rules demonstrated** (case law): @@ -86,14 +94,16 @@ Isolated contexts matter: the `lint-fixer` loop's token noise stays out of your | [`examples/storify-leaf-type.md`](examples/storify-leaf-type.md) | R3, R1, R2 β€” storifying a fat function; extracting a self-validating leaf type | | [`examples/overabstraction-cidr.md`](examples/overabstraction-cidr.md) | R1 β€” when an extraction is over-abstraction (the skeptic's payload) | | [`examples/dependency-rejection.md`](examples/dependency-rejection.md) | R8 β€” dependency rejection: eliminating globals by threading dependencies | +| [`examples/anti-if-dispatch.md`](examples/anti-if-dispatch.md) | R11 β€” duplicated kind-switch β†’ interface dispatch / strategy map, plus the kept-switch rejection (the skeptic's dispatch payload) | +| [`examples/switch-to-polymorphism.md`](examples/switch-to-polymorphism.md) | R11, R6 β€” type switch over an owned interface β†’ fill-style method; the earned/sealed interface; the dependency-direction rejection | **Skills β†’ role** (thin views): | Skill | Role | |-------|------| -| [`@linter-driven-development`](skills/linter-driven-development/SKILL.md) | Meta-orchestrator β€” sequences the five phases | +| [`@linter-driven-development`](skills/linter-driven-development/SKILL.md) | Meta-orchestrator β€” sequences the five phases (plus the autonomous PREPARE sub-phase, 1.5) | | [`@code-designing`](skills/code-designing/SKILL.md) | FORWARD view β€” which rule to open at each design step (Phase 1) | -| [`@refactoring`](skills/refactoring/SKILL.md) | BACKWARD view β€” routes each linter/review failure to its owning rule's Fix pattern | +| [`@refactoring`](skills/refactoring/SKILL.md) | BACKWARD view β€” routes each linter/review failure to its owning rule's Fix pattern; preparatory mode reshapes ahead of a planned change (Phase 1.5) | | [`@pre-commit-review`](skills/pre-commit-review/SKILL.md) | Orchestrates the hunter/skeptic review (Phase 4); reports, never edits | | [`@testing`](skills/testing/SKILL.md) | The composition ladder β€” test each behavior at the lowest rung that contains it | | [`@documentation`](skills/documentation/SKILL.md) | Repo-brain author (R9) β€” behavior docs + network wiring; FEATURE mode (Phase 5) / BOOTSTRAP mode | @@ -112,6 +122,7 @@ Isolated contexts matter: the `lint-fixer` loop's token noise stays out of your |---------|---------|----------|----------------| | [`/go-ldd-autopilot`](commands/go-ldd-autopilot.md) | Full workflow (Phases 1–5) | βœ… Yes | β€” | | [`/go-ldd-quickfix [files]`](commands/go-ldd-quickfix.md) | Quality-gates loop until green (code exists) | βœ… Yes | βœ… Optional | +| [`/go-ldd-prepare [files]`](commands/go-ldd-prepare.md) | Preparatory refactoring: reshape what a planned change touches, so it lands add-only | βœ… Yes | βœ… Optional | | [`/go-ldd-analyze [files]`](commands/go-ldd-analyze.md) | πŸ” Tests + lint + review, combined report | ❌ No | βœ… Optional | | [`/go-ldd-review [files]`](commands/go-ldd-review.md) | πŸ” Commit-readiness check | ❌ No | βœ… Optional | | [`/go-ldd-status`](commands/go-ldd-status.md) | Show current phase + progress | N/A | β€” | @@ -251,7 +262,7 @@ Skills are expert consultants you can call on demand: The [`@pre-commit-review`](skills/pre-commit-review/SKILL.md) report groups findings by urgency β€” all advisory, none block a commit: - πŸ› **Bugs** β€” fail at runtime regardless of rule (fix immediately) -- πŸ”΄ **Design Debt** β€” R1, R2, R4, R5, R6, R7, R8 (fix before commit recommended) +- πŸ”΄ **Design Debt** β€” R1, R2, R4, R5, R6, R7, R8, R11, R12 (fix before commit recommended) - 🟑 **Readability Debt** β€” R3, R9, unclear naming (improves maintainability) - 🟒 **Polish** β€” minor idiomatic improvements, the skeptic's cheaper alternatives @@ -261,7 +272,7 @@ Every finding carries evidence (`file:line` + the falsifying-question answer or The plugin follows opinionated Go best practices, each with an owning rule: -**Design:** no primitive obsession (R1), self-validating types (R2), vertical slices (R5), no globals (R8), owned goroutines and guarded shared state (R10). +**Design:** no primitive obsession (R1), self-validating types (R2), vertical slices (R5), no globals (R8), owned goroutines and guarded shared state (R10), one dispatch owner per variant family β€” the Anti-IF rule (R11), closed mutation surfaces β€” no leaked aliases or unvalidated setters (R12). **Testing:** test the public API via `pkg_test` (R7), the composition ladder over the pyramid, real in-memory dependencies over mocks, no test-only interfaces (R6). **Refactoring:** storify top-level functions (R3), helpers on the placement ladder (R4), let the linter say WHAT and the rules say HOW. **Documentation:** a networked repo brain β€” each fact at its lowest rung, reachable from the root, edges pointing both ways (R9). diff --git a/go-linter-driven-development/agents/lint-fixer.md b/go-linter-driven-development/agents/lint-fixer.md index 5e33055..04130b9 100644 --- a/go-linter-driven-development/agents/lint-fixer.md +++ b/go-linter-driven-development/agents/lint-fixer.md @@ -40,7 +40,8 @@ this table: | `funlen` | rules/R3-storifying.md (via @refactoring) | | `nestif` | rules/R3-storifying.md (via @refactoring) | | `maintidx` | rules/R3-storifying.md + rules/R1-primitive-obsession.md | -| `dupl` | rules/R1-primitive-obsession.md (extract shared type/logic) | +| `dupl` | rules/R1-primitive-obsession.md (extract shared type/logic); duplicated switches on one discriminator β†’ rules/R11-conditional-dispatch.md | +| `exhaustive` (missing enum cases) | rules/R11-conditional-dispatch.md (via @refactoring) | | revive `file-length-limit`; package-size hook failures (`hooks/check-package-sizes.sh`) | rules/R5-vertical-slice.md | | `gochecknoglobals` / `gochecknoinits` | rules/R8-no-globals.md | | `ireturn` / interface lint on single-impl interfaces | rules/R6-test-only-interfaces.md | diff --git a/go-linter-driven-development/agents/overabstraction-skeptic.md b/go-linter-driven-development/agents/overabstraction-skeptic.md index db5059a..6160514 100644 --- a/go-linter-driven-development/agents/overabstraction-skeptic.md +++ b/go-linter-driven-development/agents/overabstraction-skeptic.md @@ -43,4 +43,8 @@ what a correct refutation looks like. **Bias statement:** you exist to prevent wrap-every-string over-extraction. On a marginal score, lean REFUTED β€” a missed extraction is cheaper to fix later than a -premature one is to unwind. +premature one is to unwind. Your doctrine has names β€” cite them in verdicts where +they carry the argument: *"duplication is far cheaper than the wrong abstraction"* +(Sandi Metz), *"you aren't gonna need it"* (XP), *"a little copying is better than a +little dependency"* and *"the bigger the interface, the weaker the abstraction"* +(Rob Pike). A named principle is an argument; "feels unnecessary" is not. diff --git a/go-linter-driven-development/agents/rule-hunter.md b/go-linter-driven-development/agents/rule-hunter.md index a1cc437..095b3c3 100644 --- a/go-linter-driven-development/agents/rule-hunter.md +++ b/go-linter-driven-development/agents/rule-hunter.md @@ -33,7 +33,10 @@ detection commands. Never edit files, never run tests or fixers. **Evidence protocol:** A finding exists only when a falsifying question is answered with evidence β€” `file:line` plus the offending code excerpt or command output. No -verdicts without evidence. If evidence is absent, there is no finding. +verdicts without evidence. If evidence is absent, there is no finding. Never justify +a finding by a design maxim or general principle ("tell don't ask", "law of +Demeter") β€” maxims propose, evidence disposes; only your rule's detection commands +convict. **Output β€” one block per finding:** `rule | file:line | evidence (falsifying-question answers) | proposed fix pattern (named from the rule's Fix pattern section) | effort (S/M/L)` diff --git a/go-linter-driven-development/commands/go-ldd-analyze.md b/go-linter-driven-development/commands/go-ldd-analyze.md index b343f74..d1e7d91 100644 --- a/go-linter-driven-development/commands/go-ldd-analyze.md +++ b/go-linter-driven-development/commands/go-ldd-analyze.md @@ -63,7 +63,7 @@ Search project documentation to find test and lint commands: what needs refactoring without changing anything. (The `lint-fixer` agent, which auto-fixes, is intentionally NOT used here β€” this command never edits.) 3. **Design review**: invoke `Skill(go-linter-driven-development:pre-commit-review)` - in FULL mode over the file scope. It grep-prefilters the diff against rules R1–R10, + in FULL mode over the file scope. It grep-prefilters the diff against rules R1–R12, spawns one parallel `rule-hunter` per rule with hits, runs the `overabstraction-skeptic` over every type/package-extraction proposal, and returns evidence-backed findings. It reports β€” it never edits. @@ -76,7 +76,7 @@ Merge the three gates into one report: - βœ…/❌ **Linter**: clean / error count (with file:line and the failing linter) - βœ…/⚠️ **Review**: clean / findings, categorized as the pre-commit-review report returns them: - πŸ› **Bugs** β€” fail at runtime regardless of rule (incl. R10 goroutine leaks and unguarded concurrent writes) - - πŸ”΄ **Design Debt** β€” R1, R2, R4, R5, R6, R7, R8, R10's non-crash findings (advisory) + - πŸ”΄ **Design Debt** β€” R1, R2, R4, R5, R6, R7, R8, R10's non-crash findings, R11, R12 (advisory) - 🟑 **Readability Debt** β€” R3, R9, unclear naming - 🟒 **Polish** β€” minor idiomatic improvements, the skeptic's cheaper alternatives - 🎯 **Clustered issues**: where a linter failure and a review finding land at the same diff --git a/go-linter-driven-development/commands/go-ldd-autopilot.md b/go-linter-driven-development/commands/go-ldd-autopilot.md index f40a42b..ea2b11c 100644 --- a/go-linter-driven-development/commands/go-ldd-autopilot.md +++ b/go-linter-driven-development/commands/go-ldd-autopilot.md @@ -1,21 +1,22 @@ --- name: go-ldd-autopilot -description: Start complete linter-driven autopilot workflow (Phase 1-5) +description: Start complete linter-driven autopilot workflow (Phases 1-5, incl. the autonomous PREPARE sub-phase 1.5) argument-hint: "" allowed-tools: - Skill(go-linter-driven-development:linter-driven-development) --- -**Use the Skill tool** to invoke `Skill(go-linter-driven-development:linter-driven-development)` to run the complete five-phase workflow from design through commit-ready. +**Use the Skill tool** to invoke `Skill(go-linter-driven-development:linter-driven-development)` to run the complete workflow β€” Phases 1–5 plus the autonomous PREPARE sub-phase (1.5) β€” from design through commit-ready. ⏱️ **Estimated Duration**: 5-15 minutes (depends on feature complexity and issues found) -The skill runs all five phases: +The skill runs, in order: 1. **Pre-Flight** β€” verify Go project, discover test/lint commands, list the behaviors to deliver 2. **Phase 1 DESIGN** β€” @code-designing produces a DESIGN PLAN for your approval (no code before OK) -3. **Phase 2 IMPLEMENT** β€” per behavior: RED (one failing test) β†’ GREEN (minimum code) β†’ REFACTOR (package-scoped lint + rule greps β†’ @refactoring) -4. **Phase 3 FULL LINT** β€” one full-repo run via the `lint-fixer` agent (isolated context); mechanical fixes done, design failures escalated back to Phase 2's REFACTOR via @refactoring -5. **Phase 4 REVIEW** β€” per completed slice, @pre-commit-review orchestrates parallel `rule-hunter` agents + the `overabstraction-skeptic`; advisory findings only -6. **Phase 5 SHIP** β€” @documentation, then a commit-ready summary you approve +3. **Phase 1.5 PREPARE** β€” autonomous preparatory refactoring: survey the plan's touch points, four gates decide (multiply/safe/bounded/skeptic), reshape via @refactoring in its own commit(s) β€” no pause for approval +4. **Phase 2 IMPLEMENT** β€” per behavior: RED (one failing test) β†’ GREEN (minimum code) β†’ REFACTOR (package-scoped lint + rule greps β†’ @refactoring) +5. **Phase 3 FULL LINT** β€” one full-repo run via the `lint-fixer` agent (isolated context); mechanical fixes done, design failures escalated back to Phase 2's REFACTOR via @refactoring +6. **Phase 4 REVIEW** β€” per completed slice, @pre-commit-review orchestrates parallel `rule-hunter` agents + the `overabstraction-skeptic`; advisory findings only +7. **Phase 5 SHIP** β€” @documentation, then a commit-ready summary you approve This is the full workflow β€” use for implementing features or fixes from start to finish. diff --git a/go-linter-driven-development/commands/go-ldd-prepare.md b/go-linter-driven-development/commands/go-ldd-prepare.md new file mode 100644 index 0000000..6d29f85 --- /dev/null +++ b/go-linter-driven-development/commands/go-ldd-prepare.md @@ -0,0 +1,42 @@ +--- +name: go-ldd-prepare +description: Preparatory refactoring β€” reshape the code a planned change will touch, so the feature lands add-only +argument-hint: " [files]" +allowed-tools: + - Skill(go-linter-driven-development:refactoring) + - Skill(go-linter-driven-development:code-designing) + - Skill(go-linter-driven-development:testing) + - Task +--- + +Run standalone preparatory refactoring (Fowler: "make the change easy, then make the +easy change") for a change you're about to implement β€” without entering the full +five-phase workflow. + +⏱️ **Estimated Duration**: 2–10 minutes (zero findings is the common case and takes seconds) + +**Input**: a description of the impending change ("add an SMS channel to alerts", +"extend the exporter with histogram support"), optionally scoped to files. No +description β†’ ask for one; preparation without a change in hand is just cleanup and +belongs to `/go-ldd-quickfix`. + +**Steps** (the gate definitions live in @linter-driven-development +`` β€” apply them from there, never from memory): + +1. **Locate touch points**: from the change description, identify the files, + functions, and packages the change will extend or integrate with (grep for the + named concepts; when ambiguous, invoke @code-designing briefly to sketch the + landing zone). +2. **Survey**: run the rule detection greps (`../rules/R*.md` Falsifying questions) + scoped to the touch points. +3. **Gate autonomously**: MULTIPLY β†’ SAFE β†’ BOUNDED β†’ SKEPTICIZED, per + @linter-driven-development ``. No user questions β€” the gates + decide; deferred findings are reported, not asked about. +4. **Apply survivors** via `Skill(go-linter-driven-development:refactoring)` in + ``: characterization tests first on uncovered paths, full suite + green after every move, prep work in its own commit(s). +5. **Emit the PREPARATION LOG** and state the landing shape ("adding the SMS channel + is now one new file + one ParseChannel case"). + +Use this before starting work on a known change. For fixing code that already fails +gates, use `/go-ldd-quickfix`; for a read-only assessment, `/go-ldd-analyze`. diff --git a/go-linter-driven-development/commands/go-ldd-quickfix.md b/go-linter-driven-development/commands/go-ldd-quickfix.md index 5d9aa79..937eab3 100644 --- a/go-linter-driven-development/commands/go-ldd-quickfix.md +++ b/go-linter-driven-development/commands/go-ldd-quickfix.md @@ -23,7 +23,8 @@ Execute the quality-gates loop for already-implemented code that needs cleanup. **Phase 4 β€” REVIEW** (via @pre-commit-review, per completed slice) - @pre-commit-review orchestrates parallel `rule-hunter` agents + the `overabstraction-skeptic` against the diff; it reports, never edits - Findings return categorized (Bugs / Design Debt / Readability Debt / Polish), all advisory -- Fix bugs and user-accepted findings via @refactoring, then re-invoke @pre-commit-review in INCREMENTAL mode +- πŸ”— CLUSTER entries (β‰₯2 rules converging on one anchor) are fixed design-first: @code-designing (cluster-scoped) produces one mini plan, @refactoring implements it β€” never member-by-member +- Fix bugs and user-accepted singleton findings via @refactoring, then re-invoke @pre-commit-review in INCREMENTAL mode **Loop until**: βœ… Tests pass | βœ… `LINT STATUS: green` | βœ… @pre-commit-review INCREMENTAL delta clean (or findings explicitly deferred) diff --git a/go-linter-driven-development/commands/go-ldd-status.md b/go-linter-driven-development/commands/go-ldd-status.md index 9982db8..a6eb72b 100644 --- a/go-linter-driven-development/commands/go-ldd-status.md +++ b/go-linter-driven-development/commands/go-ldd-status.md @@ -15,7 +15,7 @@ Display current implementation status: πŸ“ Current Context: - Active plan: [file path or "conversation"] - Current behavior/slice: [which behavior's TDD cycle, if mid-implementation] - - Phase: [pre-flight / 1 DESIGN / 2 IMPLEMENT (REDβ†’GREENβ†’REFACTOR) / 3 FULL LINT / 4 REVIEW / 5 SHIP] + - Phase: [pre-flight / 1 DESIGN / 1.5 PREPARE / 2 IMPLEMENT (REDβ†’GREENβ†’REFACTOR) / 3 FULL LINT / 4 REVIEW / 5 SHIP] πŸ“Š Last Results: Tests: [status + coverage] diff --git a/go-linter-driven-development/examples/anti-if-dispatch.md b/go-linter-driven-development/examples/anti-if-dispatch.md new file mode 100644 index 0000000..3a56e1e --- /dev/null +++ b/go-linter-driven-development/examples/anti-if-dispatch.md @@ -0,0 +1,258 @@ +# Anti-IF Dispatch Case: One Decision, One Owner + +Demonstrates: R11 (edges into R1, R2, R6) + +A worked study of the R11 moves on a realistic alert-notification slice: a string +discriminator inspected in three files becomes an interface chosen once at the +boundary; a single-function variance becomes a strategy map instead; and the inverse +case β€” where the skeptic kills the extraction and the switch *stays* β€” is worked to +its cheaper alternative. The compact excerpt lives in +`../rules/R11-conditional-dispatch.md`; this file is the full case law. + +## The disease: a decision with three owners + +The alert feature delivers over email, Slack, or PagerDuty. `Alert.Channel` is a raw +string (already an R1 smell), and three sites ask what it is: + +```go +// ❌ alert/send.go +func Send(a Alert) error { + switch a.Channel { + case "email": + return smtpSend(a.Recipient, renderEmail(a)) + case "slack": + return slackPost(a.Recipient, renderSlack(a)) + case "pagerduty": + return pdCreateIncident(a.Recipient, a.Summary) + default: + return fmt.Errorf("unknown channel %q", a.Channel) + } +} + +// ❌ alert/validate.go β€” drifted: pagerduty was never added here +func validRecipient(a Alert) bool { + switch a.Channel { + case "email": + return strings.Contains(a.Recipient, "@") + case "slack": + return strings.HasPrefix(a.Recipient, "#") + } + return false +} + +// ❌ alert/retry.go β€” the same decision wearing an if-chain +func retryDelay(a Alert) time.Duration { + if a.Channel == "pagerduty" { + return 0 + } + if a.Channel == "slack" { + return 5 * time.Second + } + return time.Minute +} +``` + +Why this is a defect and not a style choice: + +- **The drift already happened.** `validRecipient` returns `false` for `"pagerduty"` + β€” not by decision, but because the second copy of the switch was not in view when + the third channel landed. Duplicated discriminators drift the same way duplicated + validation predicates drift (R1's Q2). +- **Adding SMS is a scavenger hunt.** Three known sites, plus whatever a grep misses + (test helpers, a metrics label formatter). The compiler flags none of them: an + if-chain has no completeness, and a switch with a `default` swallows the new case + silently. +- **"Unknown channel" leaks everywhere.** Every switching site carries the + `default:`/fall-through arm, so every function's signature and tests carry the + maybe-unknown concept β€” the behavioral twin of R1's maybe-invalid port. +- **Nothing unit-tests in isolation.** Slack's recipient rule is only reachable by + driving `validRecipient` with a fully built `Alert`. + +## Move 1 β€” Replace Duplicated Switch with Interface Dispatch + +Define the interface from the union of what the copies do: `Send` switches on +delivery, `validRecipient` on addressing, `retryDelay` on retry policy β€” three +methods, one type per variant: + +```go +// alert/channel.go +type Channel interface { + Send(a Alert) error + ValidRecipient(recipient string) bool + RetryDelay() time.Duration +} + +// ParseChannel is the single decision point. This switch is ALLOWED β€” +// it is the one place the raw string may be inspected (R11), exactly as +// a ParseX constructor is the one place a raw value is validated (R2). +func ParseChannel(name string) (Channel, error) { + switch name { + case "email": + return Email{}, nil + case "slack": + return Slack{}, nil + case "pagerduty": + return PagerDuty{}, nil + default: + return nil, fmt.Errorf("unknown channel %q", name) + } +} +``` + +```go +// alert/slack.go β€” each variant is a leaf type owning ALL its behavior +type Slack struct{} + +func (Slack) Send(a Alert) error { return slackPost(a.Recipient, renderSlack(a)) } +func (Slack) ValidRecipient(r string) bool { return strings.HasPrefix(r, "#") } +func (Slack) RetryDelay() time.Duration { return 5 * time.Second } +``` + +`Alert` now holds a `Channel`, constructed at the boundary (the HTTP handler or +config loader calls `ParseChannel` and fails fast there). The three switching sites +collapse to method calls: + +```go +func Send(a Alert) error { return a.Channel.Send(a) } +func validRecipient(a Alert) bool { return a.Channel.ValidRecipient(a.Recipient) } +func retryDelay(a Alert) time.Duration { return a.Channel.RetryDelay() } +``` + +(And once they are one-liners, the wrappers themselves usually dissolve into their +callers β€” the story functions call the methods directly, R3.) + +What was deleted, not relocated: every `default:` arm downstream. An `Alert` that +exists holds a `Channel` that exists; "unknown channel" is unrepresentable past +`ParseChannel`. Adding SMS is now one new file (`sms.go`) plus one `case` in +`ParseChannel` β€” no existing file changes. + +**R6 check, answered explicitly:** this interface is *earned* β€” it has three +production implementations on day one. R6 forbids interfaces whose only second +implementer is a test double; a dispatch interface born with one variant "for the +future" fails that test and should stay a switch until the second variant is real. + +### The testing payoff + +Each variant unit-tests as a leaf with literals β€” no `Alert` construction, no +switch-driving: + +```go +func TestSlack_ValidRecipient(t *testing.T) { + assert.True(t, alert.Slack{}.ValidRecipient("#oncall")) + assert.False(t, alert.Slack{}.ValidRecipient("oncall")) +} + +func TestParseChannel_Unknown(t *testing.T) { + _, err := alert.ParseChannel("carrier-pigeon") + require.Error(t, err) +} +``` + +The drift bug (`pagerduty` missing from `validRecipient`) can no longer be written: +there is no second place to forget. + +## Move 2 β€” Replace If-Chain with Strategy Map + +Not every variance deserves an interface. Suppose only *rendering* varies by an +output format, in one behavioral dimension: + +```go +// ❌ before: if-chain in the middle of business logic +func render(a Alert, format string) string { + if format == "json" { + return renderJSON(a) + } + if format == "text" { + return renderText(a) + } + return renderMarkdown(a) // silent default β€” is "yaml" markdown? nobody decided +} +``` + +One behavior β†’ a map, not three types: + +```go +type Format string + +const ( + FormatJSON Format = "json" + FormatText Format = "text" + FormatMarkdown Format = "markdown" +) + +var renderers = map[Format]func(Alert) string{ + FormatJSON: renderJSON, + FormatText: renderText, + FormatMarkdown: renderMarkdown, +} + +func ParseFormat(raw string) (Format, error) { + if _, ok := renderers[Format(raw)]; !ok { + return "", fmt.Errorf("unknown format %q", raw) + } + return Format(raw), nil +} + +func render(a Alert, f Format) string { return renderers[f](a) } +``` + +The lookup *is* the dispatch; the comma-ok check lives once, in `ParseFormat`, at the +boundary. The silent markdown default β€” an undecided decision β€” became an explicit +error. (The map is package-level immutable data, the sanctioned shape under R8; +naming the enum is R1's "Name enum strings" move.) + +## Move 3 β€” the rejection: when the switch stays + +The skeptic's side of R11, worked honestly. The same codebase has this: + +```go +// alert/severity.go β€” the ONLY site that inspects Severity +func (s Severity) Color() string { + switch s { + case SeverityInfo: + return "blue" + case SeverityWarning: + return "yellow" + case SeverityCritical: + return "red" + } + return "" +} +``` + +A dispatch-happy reading says: three variants, extract `type Severity interface` +with `Info`, `Warning`, `Critical` types. Score it before moving (R1 scorecard, via +the over-abstraction skeptic): + +- Duplication of the discriminator: **1 site** (grep `switch .*Severity` β†’ one hit) β€” +0 +- Behavioral variance: one method, returns a constant string β€” trivial β€” +0 +- Would the interface be earned (R6)? Three implementations, but each is an empty + struct wrapping one literal β€” ceremony + +Verdict: **REFUTED.** The extraction would turn 12 readable lines into three files +and an interface for zero deletion β€” no duplicated switch exists to delete. The +cheaper alternative is R11's sanctioned form, **Keep the Single Exhaustive Switch**: + +```go +func (s Severity) Color() string { + switch s { // exhaustive: linter fails the build when a Severity is added unhandled + case SeverityInfo: + return "blue" + case SeverityWarning: + return "yellow" + case SeverityCritical: + return "red" + } + return "" +} +``` + +with `exhaustive` enabled in `.golangci.yaml`. Now the linter provides what dispatch +would have: adding `SeverityFatal` fails the build at this switch instead of falling +through to `""`. That is the whole benefit, at none of the cost. + +**The dividing line, restated:** dispatch is bought with the *deletion of duplicated +decisions*. Three sites collapsed to one boundary β€” clear win (Move 1). One +single-dimension variance β€” a map (Move 2). One site, trivial variance β€” the switch +stays, made exhaustive (Move 3). If nothing gets deleted, the abstraction is +ceremony. diff --git a/go-linter-driven-development/examples/switch-to-polymorphism.md b/go-linter-driven-development/examples/switch-to-polymorphism.md new file mode 100644 index 0000000..6810d1b --- /dev/null +++ b/go-linter-driven-development/examples/switch-to-polymorphism.md @@ -0,0 +1,258 @@ +# Switch-to-Polymorphism Case: The Ever-Growing Export Switch + +Demonstrates: R11, R6 (edges into R3) + +Adapted from production code. `../examples/anti-if-dispatch.md` works R11's canonical +disease β€” a *raw* discriminator (a kind string) inspected at three sites. This case +is the type-switch sibling: a value that is **already polymorphic** (an interface, +dispatched once at construction) gets *un-dispatched* by a type switch that unpacks +its fields. The decision was made when the value was built; the switch asks it again. + +Two things make this case worth its own file. First, the *obvious* refactoring +(extract each case body into a helper) is a trap β€” it shrinks the function but +preserves the disease. Second, the rejection axis here is different from +`anti-if-dispatch.md`'s Move 3: there the skeptic kills an extraction on juiciness +(one site, trivial variance); here the counter is **dependency direction** β€” a +situation where the dispatch move is physically unavailable and the switch is the +honest answer. + +## Before β€” the hump that grows forever + +`Patch` is an interface with one method (`Type()`) and four concrete +implementations, one per export destination. The converter interrogates each +concrete type and shovels its fields into a flat wire request: + +```go +func fromUpdateArg(arg UpdateArg) updateExportRequest { + req := updateExportRequest{Name: arg.Name} + req.Type = arg.Patch.Type().String() + + switch p := arg.Patch.(type) { + case SplunkPatch: + if p.Token != nil { + s := string(*p.Token) + req.Token = &s + } + case S3Patch: + req.S3Bucket = p.Bucket + req.S3Key = p.Key + req.S3Region = p.Region + if p.Secret != nil { + s := string(*p.Secret) + req.S3Secret = &s + } + case KafkaPatch: + req.KafkaTopic = p.Topic + req.KafkaUseSASL = p.UseSASL + req.KafkaSASLUsername = p.Username + req.KafkaKeyField = p.KeyField + if p.Mechanism != nil { + m := p.Mechanism.String() + req.KafkaSASLMechanism = &m + } + if p.Password != nil { + s := string(*p.Password) + req.KafkaSASLPassword = &s + } + case SyslogPatch: + if p.Mode != nil { + m := p.Mode.String() + req.SyslogMode = &m + } + if p.RFC != nil { + r := p.RFC.String() + req.SyslogRFC = &r + } + if p.Facility != nil { + f := p.Facility.String() + req.SyslogFacility = &f + } + } + + req.setTLS(arg.TLS.Expand()) + return req +} +``` + +Three defects, and only one of them is size: + +- **The decision is asked twice (R11).** Whoever constructed `UpdateArg` already + chose `KafkaPatch` β€” the value is an interface *because* that decision was made. + The type switch re-asks it. A type switch over an interface the same package owns + is always a second ask; "decide once at the edge" was violated the moment the + cases appeared. +- **Ask-and-unpack.** The knowledge of *how a Splunk patch serializes* lives in the + consumer, not on `SplunkPatch`. Each variant's wire mapping has no owner. +- **Silent growth failure.** Adding a `PubSubPatch` and forgetting this switch + compiles clean and ships a request carrying only `Name` and `Type` β€” a runtime + no-op with no compiler, linter, or test to catch it unless someone remembers to + write one. (Mixed in, an R3 note: the business flow β€” identity β†’ payload β†’ TLS β€” + is buried under nil-deref-convert plumbing repeated nine times.) + +## The tempting wrong fix β€” extract each case body + +The reflexive move is Extract Function per case: + +```go +case KafkaPatch: + fillKafka(&req, p) +case SyslogPatch: + fillSyslog(&req, p) +``` + +The function gets shorter and each case reads better β€” and nothing real changed. +The switch still exists, still grows a case per destination forever, and a +forgotten case is still a silent no-op. This is the ceiling of function +extraction, not the fix. + +The falsifying question that breaks the frame: **why are we switching on type and +extracting data at all?** A type switch whose cases all do the same *kind* of work +(map my fields onto that struct) is behavior asking to live on the types. The +cased types already share an interface β€” the switch is a hand-rolled vtable. + +## After β€” the interface owns the behavior + +Add the fill behavior to the interface the concrete types already implement: + +```go +// Patch is implemented by each export-destination patch type. +// fillUpdate writes the destination-specific fields onto the wire request; +// shared fields (Name, Type, TLS) belong to the caller. +type Patch interface { + Type() ExportType + fillUpdate(req *updateExportRequest) +} +``` + +The orchestrator collapses to a three-beat story (R3): identity, payload, TLS. + +```go +func fromUpdateArg(arg UpdateArg) updateExportRequest { + req := updateExportRequest{ + Name: arg.Name, + Type: arg.Patch.Type().String(), + } + arg.Patch.fillUpdate(&req) + req.setTLS(arg.TLS.Expand()) + return req +} +``` + +Each destination owns its own mapping, in its own file (`splunk.go`, `s3.go`, +`kafka.go`, `syslog.go`): + +```go +func (p SplunkPatch) fillUpdate(req *updateExportRequest) { + req.Token = optSecret(p.Token) +} + +func (p S3Patch) fillUpdate(req *updateExportRequest) { + req.S3Bucket = p.Bucket + req.S3Key = p.Key + req.S3Region = p.Region + req.S3Secret = optSecret(p.Secret) +} + +func (p KafkaPatch) fillUpdate(req *updateExportRequest) { + req.KafkaTopic = p.Topic + req.KafkaUseSASL = p.UseSASL + req.KafkaSASLUsername = p.Username + req.KafkaKeyField = p.KeyField + req.KafkaSASLMechanism = optStringer(p.Mechanism) + req.KafkaSASLPassword = optSecret(p.Password) +} + +func (p SyslogPatch) fillUpdate(req *updateExportRequest) { + req.SyslogMode = optStringer(p.Mode) + req.SyslogRFC = optStringer(p.RFC) + req.SyslogFacility = optStringer(p.Facility) +} +``` + +Two tiny helpers kill the repeated nil-deref-convert dance that padded every case: + +```go +// optStringer converts an optional enum to its optional wire-string form. +func optStringer[T fmt.Stringer](v *T) *string { + if v == nil { + return nil + } + s := (*v).String() + return &s +} + +// optSecret unwraps an optional Secret for the wire request. +func optSecret(s *Secret) *string { + if s == nil { + return nil + } + v := string(*s) + return &v +} +``` + +An insert path is the same move: `fillInsert(req *insertExportRequest)` on the same +interface, and `fromInsertArg` becomes the same three-beat story. + +## The payoffs + +1. **Compile-time enforcement replaces a silent no-op.** A new `PubSubPatch` + without `fillUpdate` no longer builds. The growth failure mode moved from + "runtime request missing its payload" to "compiler error at the moment of + authorship" β€” the strongest possible catch point. (This is R11's exhaustiveness + payoff without the `exhaustive` linter: interface satisfaction *is* the + completeness proof.) +2. **Adding a destination is a new file, not an edit.** `fromUpdateArg` is frozen + at three beats; the switch version grows a hump per destination forever. +3. **The story survives (R3).** The orchestrator states *what* happens; each + destination's *how* lives one level down, on the type that owns the data. +4. **The interface is earned, and sealed.** Four production implementations β€” this + passes R6's earned-interface test (contrast: an interface whose only second + implementer is a test double). The unexported method is a bonus: no code outside + the package can implement `Patch`, so the implementation set is closed and the + compiler-enforcement guarantee in payoff 1 cannot be bypassed. + +## Fill, don't construct + +Note the method signature: `fillUpdate(req *updateExportRequest)`, not +`ToUpdateRequest() updateExportRequest`. The request carries fields the patch does +not own β€” `Name`, `Type`, TLS come from the surrounding argument. A constructor +method would either return a partial request the caller must merge (field-by-field +merging re-creates the original mess) or need the rest of the argument passed in +(the patch learns about its container). Filling keeps ownership honest: the caller +owns the shared fields, each patch owns its own. + +## The boundary counter β€” when the switch must stay + +This move has one precondition: **the package that owns the case types must also +legitimately own the output format.** Here both `Patch` and `updateExportRequest` +live in one package (a client whose API surface and wire format are the same +concern), so the method is natural. + +When the patch types live in a shared API package and the wire request is one +consumer's private detail, the move is unavailable and wrong: + +- Physically: an interface method cannot reference another package's unexported + type, and exporting the wire type just to enable the method inverts the + dependency. +- Architecturally: with multiple consumers (CLI, gateway, store), per-consumer + `fillRequest` methods accrete every consumer's serialization onto the domain + types β€” interface pollution from the opposite direction. + +In that situation the type switch at the consumer's boundary is idiomatic Go β€” the +honest tax of keeping the domain package transport-ignorant, and precisely the +boundary-adapter exemption in R11's falsifying questions. Then, and only then, the +"tempting wrong fix" above becomes the right ceiling: shrink the switch to pure +dispatch (one `fillKafka(&req, p)`-style converter per case, zero inline +field-fiddling) and stop. + +Note this rejection is orthogonal to the juiciness rejection in +`anti-if-dispatch.md` Move 3: there the extraction *could* be written but isn't +worth it; here it *cannot* be written where it belongs, at any price. + +The decision test, two questions in order: + +1. *Why am I switching on type and unpacking fields?* β†’ the behavior wants to live + on the types (R11). +2. *Does the types' package own this output format?* β†’ yes: interface method, the + switch dies. No: thin dispatch switch, and the boundary earns its keep. diff --git a/go-linter-driven-development/maxims.md b/go-linter-driven-development/maxims.md new file mode 100644 index 0000000..1f05cb8 --- /dev/null +++ b/go-linter-driven-development/maxims.md @@ -0,0 +1,226 @@ +# Maxims β€” the layer above the rules + +Rules (`rules/R1–R12`) are **compiled judgment**: each has a detection command, a +violation criterion, and a fix pattern β€” an agent can convict with evidence. A maxim +is the **question that generates such answers** in situations no rule anticipated. +"Tell, don't ask" existed before R11; R11 is what it looks like compiled for +kind-conditionals. This file holds the questions β€” both the ones already compiled +(with pointers to their rules) and the ones still uncompiled, waiting to earn +detection commands. + +## The contract: maxims propose, evidence disposes + +Maxims live where the plugin exercises **judgment**; they are banned where it must +produce **evidence**: + +- **@code-designing** interrogates the plan with these questions β€” design happens + before a diff exists, so questions are the only tool available there. +- **@refactoring's escalation path** uses them as vocabulary for *why* code resists + ("every caller asks this struct three questions and then decides β€” the design + wants Tell-Don't-Ask"). +- **The over-abstraction skeptic** cites the abstraction-economics maxims by name in + its verdicts. +- **Rule hunters NEVER cite maxims.** A finding exists only as a rule violation with + `file:line` evidence. A maxim may generate a hypothesis; only a rule's detection + command can convict. This is what keeps the review credible. + +**Graduation:** when a maxim keeps generating findings that no rule can express, that +is the signal to compile it β€” write the R-file, give it falsifying questions with +detection commands, and move its entry here from *uncompiled* to *compiled*. R11 and +R4's feature-envy question are graduates of exactly this path. + +--- + +## Behavior and ownership + +### Tell, don't ask +β€” Andy Hunt & Dave Thomas, *The Pragmatic Programmer* + +**Ask:** what will the caller *do* with the value it is requesting β€” and does that +decision belong on the type that owns the value? + +**Compiled into:** `rules/R11-conditional-dispatch.md` (asking what a value *is*), +`rules/R4-helper-placement.md` Q6 (feature envy β€” asking for data to decide with). + +### Talk to your friends, not your friends' friends +β€” The Law of Demeter (Karl Lieberherr) + +**Ask:** is this caller navigating a path (`a.B().C().D()`) it has no business +knowing? First apply Tell-Don't-Ask β€” moving the *behavior* usually dissolves the +chain. What survives is data egress at a boundary, where a one-shot adapter mapping +is the honest form. + +**Compiled into:** `rules/R4-helper-placement.md` β€” Q6 (feature envy) plus the +message-chain and middle-man bullets in its Fix pattern (Hide Delegate subordinated +to Tell-Don't-Ask; Remove Middle Man as the per-method ceremony verdict; the +domain-type embedding trap). Uncompiled residue: detection commands for +forward-heavy types and boundary-crossing chains β€” graduates to an R4 falsifying +question if the hunter keeps stumbling over them. + +## State and construction + +### Make illegal states unrepresentable +β€” Yaron Minsky + +**Ask:** can this type hold a value its methods would have to defend against? Delete +the possibility, not the symptom. + +**Compiled into:** `rules/R2-self-validating-types.md`, and its aliasing corollary +`rules/R12-mutation-discipline.md` (a leaked internal reference re-legalizes the +illegal state). + +### Parse, don't validate +β€” Alexis King + +**Ask:** does this check produce a *more-typed value* (`ParseX(raw) (X, error)`), or +just a boolean the next caller must remember? Validation that returns proof is +parsing; validation that returns advice is a latent re-check. + +**Compiled into:** `rules/R2-self-validating-types.md`; `rules/R3-storifying.md` +Split Phase (phase 1 as the parse). + +### Make the zero value useful +β€” Rob Pike, Go Proverbs + +**Ask:** could `var x T` just work (`bytes.Buffer`, `sync.Mutex`)? Held in +deliberate tension with R2: **mechanism types** earn zero-value usefulness; +**validated domain types** earn constructors β€” a type that needs invariants cannot +also promise a useful zero. Decide which family a new type is in; don't split the +difference. + +**Uncompiled** β€” lives here as a design-time question. + +## Abstraction economics + +### Every indirection must earn its keep +β€” this plugin's own synthesis (the generalized juiciness test) + +**Ask:** what does this indirection *own* β€” a validation, a decision, a second +production implementation, a deleted duplication, a real race, a real escaping +alias? If the answer is nothing, it is ceremony: delete it. + +**Compiled into:** every inverse trap in the rule set β€” one principle at six +granularities: type (R1's scorecard and ceremony wrappers), interface (R6's +earned-interface test), method (R4's middle-man bullet), dispatch (R11's unearned +abstractions), guard (R10's decorative mutexes), copy (R12's ceremony copies). The +`overabstraction-skeptic` is its enforcement agent. Nuance: the R1 scorecard is the +*prospective* form (score before the type is born); the inverse traps are the +*retrospective* form (this indirection exists β€” does it still own anything?). + +### Duplication is far cheaper than the wrong abstraction +β€” Sandi Metz + +**Ask:** is this extraction *earning* its indirection today, with the callers in +hand β€” or is it a bet on imagined futures? + +**Compiled into:** the `overabstraction-skeptic` agent (its bias statement is this +maxim), `rules/R1-primitive-obsession.md`'s inverse trap and scorecard. + +### You aren't gonna need it (YAGNI) +β€” Extreme Programming + +**Ask:** does anything *present* require this flexibility? + +**Compiled into:** `rules/R6-test-only-interfaces.md` (an interface is earned by a +second production implementation, never by "for the future"). + +### Three strikes and you refactor +β€” Don Roberts, via *Refactoring* + +**Ask:** how many real occurrences exist *right now*? One is an instance, two is a +coincidence, three is a pattern. + +**Compiled into:** `rules/R1-primitive-obsession.md` scorecard usage points. + +### A little copying is better than a little dependency +β€” Rob Pike, Go Proverbs + +**Ask:** does sharing these four lines couple two features that would otherwise +evolve independently? Promotion to a shared package is a *dependency*, and +dependencies cost more than duplication until the third strike. + +**Compiled into:** partially the skeptic + `rules/R4-helper-placement.md`'s ladder. +Uncompiled residue: the explicit copy-first default for tiny cross-feature helpers. + +### The bigger the interface, the weaker the abstraction +β€” Rob Pike, Go Proverbs + +**Ask:** could this interface be one method (`io.Reader`)? Would a consumer with +half the methods still satisfy every caller? + +**Compiled into:** partially `rules/R6-test-only-interfaces.md` ("small and +cohesive"). Uncompiled residue: earned interfaces that accrete methods. + +## Process and economics + +### Make the change easy, then make the easy change +β€” Kent Beck + +**Ask:** does the code's current shape fight the change in hand? Reshape first, in +its own commit β€” bounded by what the change touches. + +**Compiled into:** @linter-driven-development `` and +@refactoring ``. + +### If a test is hard to write, the design is wrong +β€” Steve Freeman & Nat Pryce, *Growing Object-Oriented Software* + +**Ask:** what is the test's pain telling you? Huge fixtures β†’ the unit is too big; +global mutation β†’ a seam is missing; mocks everywhere β†’ the boundaries are wrong. +Never silence test pain with test machinery. + +**Compiled into:** the RED-friction escape hatch (@linter-driven-development +Phase 2), `rules/R6-test-only-interfaces.md`, `rules/R7-test-placement.md`. + +### If it hurts, do it more often +β€” Martin Fowler + +**Ask:** is this pain a batch-size problem? Deferred checks compound; per-cycle +checks stay trivial. + +**Compiled into:** the five-phase cadence itself β€” package-scoped lint every +REDβ†’GREENβ†’REFACTOR cycle instead of one bulk reckoning at the end. + +### Premature optimization is the root of all evil +β€” Donald Knuth + +**Ask:** is there a profile? Clarity first; optimize the measured 3%, never the +imagined 30%. (R12's ceremony-copy inverse is one instance: don't "optimize" *or* +"defend" without evidence of need.) + +**Uncompiled** β€” no rule owns profile-first optimization discipline yet. + +## Clarity and knowledge + +### Clear is better than clever +β€” Rob Pike, Go Proverbs + +**Ask:** will a reader get this in 10–15 seconds? Cleverness is a cost paid by every +future reader. + +**Compiled into:** `rules/R3-storifying.md` and the plugin's readability philosophy. + +### Once and only once +β€” Kent Beck (and DRY: "every piece of knowledge has a single, unambiguous, +authoritative representation" β€” Hunt & Thomas) + +**Ask:** how many places own this fact? Note it is about *knowledge*, not lines: +two similar-looking blocks encoding different decisions are not duplication, and +one decision spread across five switches is (R11) β€” count owners, not text. + +**Compiled into:** `rules/R1-primitive-obsession.md` Q2, +`rules/R11-conditional-dispatch.md`, and this plugin's own architecture contract +("one fact per fact"). + +## Architecture + +### Depend in the direction of stability +β€” Robert C. Martin + +**Ask:** which side of this boundary changes more often β€” and does the arrow point +from volatile to stable? A move that makes a stable package import a volatile +consumer is wrong even when it looks cleaner. + +**Compiled into:** `rules/R8-no-globals.md` (downward imports), +`examples/switch-to-polymorphism.md`'s dependency-direction rejection (via +`rules/R11-conditional-dispatch.md`). diff --git a/go-linter-driven-development/rules/R1-primitive-obsession.md b/go-linter-driven-development/rules/R1-primitive-obsession.md index d8dc717..d474202 100644 --- a/go-linter-driven-development/rules/R1-primitive-obsession.md +++ b/go-linter-driven-development/rules/R1-primitive-obsession.md @@ -256,6 +256,11 @@ Stage 2 shows it applied. absence/invalidity β†’ `(X, bool)` or `(X, error)`. - **Name enum strings**: `if status == "READY"` β†’ `type Status string` with `const StatusReady Status = "READY"`. +- **Introduce Parameter Object** (Fowler): the same group of parameters traveling + through multiple signatures (`host string, port int, useTLS bool`) becomes one + type β€” that is the scorecard's "grouping related data that travels together" made + concrete. Prefer passing the whole object over re-exploding its fields at the next + call (Preserve Whole Object). - **Over-abstraction found instead?** Apply the cheaper alternative β€” better naming, or private fields + accessors β€” per `../examples/overabstraction-cidr.md`. - Multi-rule refactoring procedure (sequencing extraction with storifying): @@ -289,7 +294,15 @@ Answer each with evidence (`file:line`, command output) β€” never a bare verdict whose signature has no `bool` or `error` result. Violation: validity encoded in-band β€” requires comma-ok or `(X, error)`. -5. **Inverse β€” is a NEW type in the diff mere ceremony?** +5. **Do the same parameters travel together across signatures?** + Detection: for each changed function with β‰₯3 parameters, grep the package for the + same parameter-name pair/trio in other signatures, e.g. + `grep -rnE 'func .*host string.*port int' --include='*.go' .` + Violation: the same group of β‰₯2–3 parameters co-occurs in β‰₯2 signatures β€” a data + clump; Introduce Parameter Object (score it: grouping-that-travels is +2 on the + scorecard, plus its usage points). + +6. **Inverse β€” is a NEW type in the diff mere ceremony?** Detection: count its methods (`grep -c 'func ([a-z0-9]* *\*\?)' `) and check whether any method does more than unwrap or rename the primitive; score it with the scorecard above. diff --git a/go-linter-driven-development/rules/R11-conditional-dispatch.md b/go-linter-driven-development/rules/R11-conditional-dispatch.md new file mode 100644 index 0000000..f2890c9 --- /dev/null +++ b/go-linter-driven-development/rules/R11-conditional-dispatch.md @@ -0,0 +1,235 @@ +# R11 β€” Conditional Dispatch (Anti-IF) + +## Principle + +A conditional that asks what a value *is* β€” a type switch, or a switch/if-chain on a +kind/status/mode discriminator β€” may exist **once**. The second copy of that +discriminator is a missing polymorphic type: the variants want to be implementations +of an interface (or entries in a dispatch map), chosen once at the boundary, so +downstream code *tells* the value what to do instead of asking what it is. One +well-placed, exhaustive switch is not a defect; a duplicated one always is. + +## Why + +Every `if (new kind) { new code }` doubles the execution paths through the function β€” +five conditionals means 32 paths to reason about and test. Worse, kind-switches +replicate: the same `switch msg.Channel` appears in send, validate, format, and retry +code, and adding a variant means finding and editing every copy β€” the one you miss is +the bug that ships. The compiler cannot help: an if-chain has no notion of +completeness, so a forgotten variant falls through silently. Dispatching once β€” +constructing the right implementation at the boundary (`R2-self-validating-types.md` +owns "validate once at the edge"; this rule is its behavioral twin: *decide* once at +the edge) β€” collapses N switches into one construction site, makes each variant a +leaf that unit-tests in isolation, and turns "add a variant" into "add a type" with +zero edits to existing code. This idea comes from the Anti-IF movement (Cirillo, +2007): the enemy is not `if`, it is the duplicated kind-conditional. + +## Canonical example + +A notifier must deliver alerts over email, Slack, or PagerDuty. The channel is decided +by a string field, and three parts of the codebase ask which one it is. + +### Before + +```go +// ❌ alert/send.go β€” first copy of the discriminator +func Send(a Alert) error { + switch a.Channel { + case "email": + return smtpSend(a.Recipient, renderEmail(a)) + case "slack": + return slackPost(a.Recipient, renderSlack(a)) + case "pagerduty": + return pdCreateIncident(a.Recipient, a.Summary) + default: + return fmt.Errorf("unknown channel %q", a.Channel) + } +} + +// ❌ alert/validate.go β€” second copy, drifting already: nobody added pagerduty here +func validRecipient(a Alert) bool { + switch a.Channel { + case "email": + return strings.Contains(a.Recipient, "@") + case "slack": + return strings.HasPrefix(a.Recipient, "#") + } + return false +} + +// ❌ alert/retry.go β€” third copy, as an if-chain this time +func retryDelay(a Alert) time.Duration { + if a.Channel == "pagerduty" { + return 0 + } + if a.Channel == "slack" { + return 5 * time.Second + } + return time.Minute +} +``` + +Three owners of one decision, already inconsistent: `validRecipient` silently returns +`false` for PagerDuty because the second copy was never updated. Adding SMS means +finding all three (and the fourth one hiding in a test helper). Every function also +carries the `default:` error path β€” the "maybe-unknown channel" concept leaks into +each call site, the behavioral twin of R1's maybe-invalid port. + +### After + +```go +// Channel is the behavior, not a string. Each variant is a leaf type. +type Channel interface { + Send(a Alert) error + ValidRecipient(recipient string) bool + RetryDelay() time.Duration +} + +// ParseChannel is the ONLY place the raw string is inspected β€” +// the decision is made once, at the boundary, like R2's ParsePort. +func ParseChannel(name string) (Channel, error) { + switch name { + case "email": + return Email{}, nil + case "slack": + return Slack{}, nil + case "pagerduty": + return PagerDuty{}, nil + default: + return nil, fmt.Errorf("unknown channel %q", name) + } +} + +type Slack struct{} + +func (Slack) Send(a Alert) error { return slackPost(a.Recipient, renderSlack(a)) } +func (Slack) ValidRecipient(r string) bool { return strings.HasPrefix(r, "#") } +func (Slack) RetryDelay() time.Duration { return 5 * time.Second } +``` + +The three switches are gone β€” call sites read `a.Channel.Send(a)`, +`a.Channel.RetryDelay()`. There is no `default:` anywhere downstream: an `Alert` that +exists holds a `Channel` that exists, so "unknown channel" is unrepresentable past +the boundary. Adding SMS is one new type plus one `case` in `ParseChannel` β€” existing +files untouched, and each channel's behavior unit-tests as a leaf with literals. +Full worked study including the strategy-map variant and the rejection counter-case: +`../examples/anti-if-dispatch.md`. + +## Design guidance + +- **The trigger is duplication, not existence.** Count the sites that inspect the same + discriminator. One site β€” keep the switch (make it exhaustive). Two or more β€” + the variants are a type family; dispatch. +- **Decide once, at the edge.** The one legitimate inspection of the raw discriminator + is the constructor/parser that picks the implementation + (`R2-self-validating-types.md` for the constructor discipline). Downstream code + holds the chosen behavior and never re-asks. The corollary: a type switch over an + interface the same package owns is always a re-ask β€” the decision was made when + the value was constructed; cases that unpack the variants' fields are behavior + asking to live on the interface (`../examples/switch-to-polymorphism.md`). +- **Dispatch requires owning the output.** An interface method can only be written + in the package that declares the interface, and it cannot reference another + package's unexported types. When the switch's output format belongs to a consumer + (a private wire request in a client package) and the variants live in a shared API + package, the move is unavailable β€” and forcing it (exporting the wire type, + per-consumer `fillRequest` methods on domain types) inverts the dependency. + There the switch is the honest boundary tax: shrink it to pure dispatch (one + converter call per case) and stop. Worked counter-case, including the fill-style + method shape for when the move IS available: + `../examples/switch-to-polymorphism.md`. +- **Interface vs strategy map.** Variants with several behaviors or state β†’ interface + with one type per variant. Variants that differ by a single function β†’ a map + (`var renderers = map[Format]func(Alert) string{...}`) β€” a map lookup with a + comma-ok check is a dispatch, not a conditional. Either way the decision has one + owner. +- **Null object over nil-checks.** A scattered `if x != nil { x.Log(...) }` is the + same disease with two variants. Construct a do-nothing implementation + (`type NopLogger struct{}`) once; delete every guard. (R2's "nil is not a value" + covers the constructor side.) +- **Flag arguments are two functions.** `func Render(a Alert, short bool)` forces + every caller through a conditional the callee then unpicks. Split into `Render` and + `RenderShort`, or make the variant a type. +- **A kept switch must be exhaustive.** When one switch over a closed enum stays + (single site, trivial variance), name the enum (`R1-primitive-obsession.md`, + "Name enum strings"), drop the `default`, and let the `exhaustive` linter prove + completeness β€” the linter then does what the if-chain never could: fail the build + when a variant is added but not handled. +- **The over-abstraction trap, dispatch edition.** An interface with one production + implementation is R6's territory; two trivial implementations behind one switch at + one site score LOW on R1's juiciness scorecard β€” keep the conditional. Conditionals + on *state/values* (`if n > threshold`, `if err != nil`, guard clauses per + `R3-storifying.md`) are healthy control flow, not dispatch β€” this rule never + touches them. + +## Fix pattern + +- **Replace Duplicated Switch with Interface Dispatch**: define the interface from the + union of what all copies of the switch do (one method per switching site is a + starting point, then collapse); one type per variant; move each `case` body into + its variant; introduce `ParseX(raw) (X, error)` as the single decision point and + migrate call sites to method calls. When the dispatch produces an output that + carries fields the variants don't own (shared name/TLS on a wire request), give + the interface a fill-style method (`fillUpdate(req *T)`) instead of a constructor β€” + the caller owns the shared fields, each variant fills its own + (`../examples/switch-to-polymorphism.md`). +- **Replace If-Chain with Strategy Map**: single-behavior variance β†’ package-level + `map[Kind]func(...)` (or a field), comma-ok on lookup at the boundary only. +- **Introduce Null Object**: absent-collaborator nil-checks β†’ a no-op implementation + constructed by default; delete the guards. +- **Split Flag Argument**: boolean/enum parameter that selects behavior β†’ two named + functions, or a variant type chosen by the caller's constructor. +- **Keep the Single Exhaustive Switch**: one site, closed enum β†’ named enum type (R1), + no `default`, `exhaustive` linter enforcing completeness. This is the rule's + sanctioned form β€” record it as the decision, not a TODO. +- New types this creates must pass R1's juiciness scorecard, land per + `R4-helper-placement.md`, and never become test-only interfaces + (`R6-test-only-interfaces.md`). Rejection case law β€” juiciness (the switch stays, + goes exhaustive): `../examples/anti-if-dispatch.md`; dependency direction (the + move is unavailable across the package boundary): + `../examples/switch-to-polymorphism.md`. + +## Falsifying questions + +Answer each with evidence (`file:line`, command output) β€” never a bare verdict. + +1. **Is the same discriminator inspected in more than one place?** + Detection: list discriminators in the diff β€” + `grep -nE 'switch [a-zA-Z_.]+\.(Type|Kind|Status|Mode|Channel|Format|Level)\b' $(git diff --name-only -- '*.go')` + and if-chain forms `grep -nE 'if [a-zA-Z_.]+\.(Type|Kind|Status|Mode|Channel|Format|Level) ==' ...`; + then count each across the package: `grep -rn 'switch .*\.' --include='*.go' . | wc -l` (plus `== ` comparisons on the same field). + Violation: β‰₯2 sites inspecting one discriminator β€” the decision has no single + owner; route to Interface Dispatch or Strategy Map. + +2. **Does a type switch dispatch on concrete types outside a boundary?** + Detection: `grep -rn 'switch .* := .*\.(type)' --include='*.go' .` β€” for each hit, + is it in a `ParseX`/decoder/boundary adapter, or in business logic? + Violation: a type switch in domain logic whose cases call variant-specific + behavior or unpack the variants' fields β€” the behavior belongs on the variants. + A switch over an interface the *same package* owns is a violation even at a + single site and even in a converter: the decision was already made at + construction, and interface satisfaction gives the completeness proof a + switch can't (`../examples/switch-to-polymorphism.md`). The boundary exemption + applies only when the output format belongs to a *different* package than the + cased types (that example's boundary counter) β€” there, the finding is limited + to shrinking the switch to pure dispatch. `errors.As`/`errors.Is` chains and + decode/unmarshal of foreign types are not this pattern. + +3. **Does a `default:` (or trailing `else`) handle "unknown kind" away from the boundary?** + Detection: for each switch found in Q1, check the `default` arm for + `errors.New`/`fmt.Errorf`/panic on an unknown-kind message. + Violation: unknown-kind errors deep in the call graph β€” the maybe-unknown concept + leaked past construction; dispatch should have been chosen at `ParseX`. + +4. **Does a boolean parameter select between behaviors?** + Detection: `grep -nE 'func .*\(.*\b(is|use|with|enable|skip)[A-Za-z]* bool' $(git diff --name-only -- '*.go')`; + check whether the function branches on it near the top. + Violation: a flag argument whose branches share little code β€” Split Flag Argument. + +5. **Inverse β€” is a NEW dispatch abstraction in the diff unearned?** + Detection: for each new interface/strategy map in the diff, count production + implementations/entries and the number of sites the old conditional occupied + (`git log -p` or the pre-diff file). + Violation: one switching site with trivial variance replaced by an interface β€” + score it (R1 scorecard); if LOW, the finding is the *extraction*, and the fix is + Keep the Single Exhaustive Switch. An interface whose second implementation exists + only in tests is an R6 violation, not a dispatch win. diff --git a/go-linter-driven-development/rules/R12-mutation-discipline.md b/go-linter-driven-development/rules/R12-mutation-discipline.md new file mode 100644 index 0000000..e35cf0d --- /dev/null +++ b/go-linter-driven-development/rules/R12-mutation-discipline.md @@ -0,0 +1,204 @@ +# R12 β€” Mutation Discipline (Encapsulated State) + +## Principle + +A validated value changes state only through methods that own its invariants β€” never +through leaked internals. Constructors copy the slices and maps they are given; +queries return copies (or iterators), not the internal reference; a method is a query +or a modifier, not both; and a type with a validating constructor exposes no setter +that skips the validation. This rule adapts Fowler's *Mutable Data* smell family +(Refactoring, 2nd ed.: Encapsulate Collection, Separate Query from Modifier, Remove +Setting Method, Split Variable) to Go, where slices and maps are references into +shared backing storage. + +## Why + +R2's payoff β€” validate once, trust the value everywhere after β€” is void the moment an +internal slice escapes. In Go, `return g.perms` does not return the permissions; it +returns a mutable alias into them. The caller can sort, truncate, or overwrite the +"validated" state without calling a single method, so no grep for setters and no +review of the type's own file will ever find the write that broke the invariant. The +same aliasing runs backward: a constructor that stores a caller's slice without +copying has handed its state to code it has never met. Setters reopen the constructor +from the side, mixed query/modifiers make every call site a potential hidden write, +and a variable reused for two meanings makes both untraceable. Mutation is not the +defect β€” *unowned* mutation is: every state change must pass through code that knows +the invariants. + +## Canonical example + +`Grants` guarantees a non-empty, deduplicated permission set β€” enforced in the +constructor per R2. + +### Before + +```go +type Grants struct { + perms []Permission // constructor guarantees: non-empty, deduplicated +} + +func ParseGrants(raw []string) (Grants, error) { + perms, err := dedupeAndValidate(raw) + if err != nil { + return Grants{}, err + } + return Grants{perms: perms}, nil +} + +// ❌ returns a mutable alias into the validated state +func (g Grants) All() []Permission { return g.perms } +``` + +```go +// ❌ a distant caller, months later +perms := user.Grants.All() +sort.Slice(perms, func(i, j int) bool { ... }) // reorders internal state +perms[0] = PermissionNone // corrupts it β€” no method called +``` + +The constructor's guarantee is now a lie, and nothing in `grants.go` changed. The +write that broke the invariant lives in a file the type's owner has never seen; no +detection aimed at the type itself can find it. The backward version is just as +silent: + +```go +// ❌ constructor stores the caller's slice +func NewSchedule(days []Weekday) (Schedule, error) { + if len(days) == 0 { + return Schedule{}, errors.New("schedule: no days") + } + return Schedule{days: days}, nil +} + +days := []Weekday{Monday} +s, _ := NewSchedule(days) +days[0] = Sunday // s just changed. NewSchedule's validation saw a different value. +``` + +### After + +```go +func ParseGrants(raw []string) (Grants, error) { + perms, err := dedupeAndValidate(raw) // freshly built here β€” no shared alias + if err != nil { + return Grants{}, err + } + return Grants{perms: perms}, nil +} + +// All returns a copy; callers may do anything with it. +func (g Grants) All() []Permission { return slices.Clone(g.perms) } + +// Or expose iteration instead of the collection (no copy, no alias). +// iter.Seq / slices.Values require Go 1.23+; on older Go, a walker method +// (func (g Grants) Each(yield func(Permission) bool)) is the same move. +func (g Grants) Each() iter.Seq[Permission] { return slices.Values(g.perms) } + +func NewSchedule(days []Weekday) (Schedule, error) { + if len(days) == 0 { + return Schedule{}, errors.New("schedule: no days") + } + return Schedule{days: slices.Clone(days)}, nil // copy on the way in +} +``` + +Now every mutation path runs through the type. The caller's `sort.Slice` reorders its +own copy; the caller's `days[0] = Sunday` changes a slice `Schedule` no longer +shares. The invariant has exactly one set of doors, and the constructor guards all of +them. + +## Design guidance + +- **Copy at both edges.** A constructor clones slice/map arguments (or builds fresh + ones, as `dedupeAndValidate` does); a query returns `slices.Clone`/`maps.Clone` or + an iterator (`iter.Seq`). Between the edges, methods mutate freely β€” that interior + is exactly what the type owns. +- **Iterators beat copies for read paths.** When callers only range, expose + `iter.Seq[T]` (or a `Each(func(T) bool)` walker) β€” no alias escapes and no copy is + paid. Return a copy only when callers legitimately need their own collection. +- **A method is a query or a modifier.** A caller who wants the value must be able to + get it without causing the side effect (Fowler: Separate Query from Modifier). + `R3-storifying.md`'s Honest Rename is the naming half β€” a mutator must sound like + one; this rule owns the structural half β€” when call sites need the query alone, + split the method in two. +- **No setters around a validating constructor.** A `SetPort(n)` that assigns without + checking is a hole in `ParsePort`'s wall. If post-construction change is a real + requirement, the mutator validates exactly as the constructor does β€” or returns a + new value (`WithPort(n) (Server, error)`). If it isn't a real requirement, there is + no setter. (`R2-self-validating-types.md` owns construction; this rule owns the + paths that could bypass it afterward.) +- **One variable, one purpose.** A variable reassigned to mean something new + (`size := len(x)` … `size = size * unitPrice`) hides a phase change inside a name. + Split it into two named variables (Fowler: Split Variable); if the phases are big, + that's `R3-storifying.md`'s extraction signal. +- **The inverse trap: ceremony copies.** Cloning a slice that never escapes the + function, or copying inside a hot loop "to be safe," is defensive noise β€” the + mirror of R1's ceremony wrappers. Copy where an alias crosses an ownership + boundary (constructor arguments, query returns on validated types), not + everywhere a slice appears. Local, short-lived sharing inside one function is + fine and idiomatic. + +## Fix pattern + +- **Copy on the Way In**: constructor stores `slices.Clone(arg)` / `maps.Clone(arg)` + (or builds its own collection) instead of the caller's reference. +- **Copy on the Way Out / Encapsulate Collection**: queries on validated types return + clones or `iter.Seq` iterators; delete call-site mutations of the returned value or + convert them into named methods on the type (`Sorted()`, `Without(p)`). +- **Separate Query from Modifier**: split a method that both returns data and mutates + into a pure query and a command; migrate each call site to the half it actually + uses. (Pure renaming cases stay with R3's Honest Rename.) +- **Remove Setting Method**: delete the setter; route the change through a validating + mutator, a `WithX` copy-constructor, or full reconstruction via `ParseX`. +- **Split Variable**: one assignment per meaning; new meaning, new name. +- New named methods this creates (`Sorted()`, `WithX`) must earn their place β€” + score against `R1-primitive-obsession.md` before adding; a method nobody calls + twice is ceremony. + +## Falsifying questions + +Answer each with evidence (`file:line`, command output) β€” never a bare verdict. + +1. **Does a method return an internal slice or map by reference?** + Detection: for each type in the diff with a validating constructor, list its + slice/map fields (`grep -A8 'type struct' `), then + `grep -nE 'return [a-z][a-zA-Z]*\.(|)$' ` β€” a bare + `return x.field` with no `Clone`/copy/iterator around it. + Violation: an internal reference escapes a validated type β€” Copy on the Way Out. + +2. **Does a constructor store a caller-provided slice/map without copying?** + Detection: inside each `ParseX`/`NewX` in the diff, check the struct literal for a + slice/map field assigned directly from a parameter identifier + (`grep -nE '\s*[,}]' within the return literal). + Violation: the type's state aliases memory the caller still holds β€” Copy on the + Way In. (A collection built inside the constructor, like `dedupeAndValidate`'s + result, is fine β€” no one else holds it.) + +3. **Does one method both return domain data and mutate the receiver?** + Detection: for each changed method with a non-error return value, + grep its body for assignments to receiver fields (`. =`, + `append(.` ). + Violation: a query/modifier hybrid where any call site discards the return value + or calls it only for the effect β€” Separate Query from Modifier. (If every caller + genuinely needs both halves atomically β€” `sync`-guarded pop-and-report β€” it is + one operation; name it as a mutator per R3 and move on.) + +4. **Can a validated type be mutated around its constructor?** + Detection: `grep -rnE 'func \([a-z][a-zA-Z]* \*?[A-Z][a-zA-Z]*\) Set[A-Z]' --include='*.go' .` + for setters; for each hit, does the receiver type have a `ParseX`/`NewX` that + validates, and does the setter re-check? + Violation: a setter that assigns unchecked on a constructor-validated type β€” + Remove Setting Method. (Exported mutable fields on such types are R2's Q1.) + +5. **Is one variable reassigned to mean something different?** + Detection: read each changed function; for every reassignment (`x = ...` after + `x := ...`), ask whether the right-hand side computes the same concept. + Violation: two meanings under one name β€” Split Variable; cite both assignments. + +6. **Inverse β€” does the diff copy defensively where no alias escapes?** + Detection: for each new `slices.Clone`/`maps.Clone`/manual copy loop in the diff, + trace the copied value: does the source or the copy ever cross a function + boundary or outlive the call? + Violation: cloning data that provably never escapes, or copying per-iteration in + a loop the profile cares about β€” ceremony; delete the copy and note why sharing + is safe. diff --git a/go-linter-driven-development/rules/R3-storifying.md b/go-linter-driven-development/rules/R3-storifying.md index 1d7d27e..71997de 100644 --- a/go-linter-driven-development/rules/R3-storifying.md +++ b/go-linter-driven-development/rules/R3-storifying.md @@ -126,6 +126,15 @@ helpers were renamed `alignIPv4`/`alignIPv6` β€” "align" admits the side effect full move, and `R1-primitive-obsession.md` to score whether the type is warranted. - **Replace Nesting with Early Returns**: invert conditions, return early, flatten to ≀2 levels. +- **Split Phase** (Fowler): when one function interleaves decoding/parsing with + computation β€” wire fields and business decisions in the same body β€” split it into + phase 1, which parses input into an intermediate domain structure, and phase 2, + which computes over that structure alone. The intermediate type is a leaf + candidate (score per `R1-primitive-obsession.md`); when phase 1 validates, it is a + `ParseX` constructor and the move collapses into `R2-self-validating-types.md`. + Split Phase differs from Extract Function: extraction names a step in place, Split + Phase introduces a data structure *between* the steps so each phase can change β€” + and be tested β€” without the other. - **Honest Rename**: mutating helpers get mutating names (`parseIP4` β†’ `alignIPv4`). - Multi-rule sequencing (storify first or extract first, and when to stop): `../skills/refactoring/reference.md`. Forward design of the new types: diff --git a/go-linter-driven-development/rules/R4-helper-placement.md b/go-linter-driven-development/rules/R4-helper-placement.md index 78ecac1..77ccd4d 100644 --- a/go-linter-driven-development/rules/R4-helper-placement.md +++ b/go-linter-driven-development/rules/R4-helper-placement.md @@ -108,6 +108,31 @@ vocabulary of one β€” fold it into the vocabulary it belongs to. A role name (`u example). - **Split policy from vocabulary during promotion**: feature constants and preference logic stay in the feature; only the domain-generic types and queries move. +- **Move Method to the Envied Type** (Fowler: Feature Envy β†’ Move Function): a + function that reads another type's data more than its own belongs on that type β€” + move it there, then place the enriched type on the ladder as usual. If the envied + type is foreign (another module's DTO), wrap it first (`R1-primitive-obsession.md`) + and hang the behavior on the wrapper. +- **A message chain is a placement signal, not a wrapper order** (Fowler: Message + Chains). Before "fixing" `order.Customer().Address().City()` by adding a + `CustomerCity()` forwarder, ask what the caller *does* with the endpoint (Tell, + Don't Ask β€” `../maxims.md`): a decision or computation β†’ that behavior moves onto + the chain's owner (Move Method to the Envied Type, above), where the chain + collapses into a one-hop walk of the type's own composition β€” which was never the + problem. Only data egress at a boundary (rendering, serialization, wire mapping) + legitimately keeps the chain, and there it lives as a one-shot mapping inside the + adapter, not scattered through domain code. +- **A type speaks for its parts only when it has something to add** (Fowler: Middle + Man). A method whose entire body is `return o.x.Method()` β€” no decision, no + combination, no invariant β€” is R1's ceremony verdict applied per method: the + indirection owns nothing, so it goes. A type whose surface is mostly such forwards + is a worse copy of its field's API β€” delete the forwards and hand callers the part + (`o.Customer()`), keeping only delegations that carry a rule (`ShippingAddress()` + choosing gift recipient over buyer earns its place; `CustomerEmail()` does not). + The Go accelerant: embedding a domain type (`type Order struct{ Customer }`) + manufactures this smell in one line by promoting the entire foreign API onto the + outer type β€” embed for genuine is-a (interface embedding, `sync` primitives per + `R10-concurrency-safety.md`), never to save typing `o.customer.`. - Multi-rule extraction sequencing: `../skills/refactoring/reference.md`. Forward design of the promoted package: @code-designing. @@ -146,3 +171,12 @@ Answer each with evidence (`file:line`, command output) β€” never a bare verdict Violation: any feature-specific literal or preference decision inside a domain-generic package β€” policy stays in the feature (Stage 2 of `R1-primitive-obsession.md`). + +6. **Does a changed function envy another type's data?** + Detection: for each changed function/method, count field/method accesses per + value: `grep -o '\.[a-zA-Z]*' | sort | uniq -c` versus the same + count for its most-touched parameter or field. + Violation: accesses on one foreign value outnumber accesses on the receiver (or + on all local data, for a free function) and the foreign type is yours to extend β€” + Move Method to the Envied Type, then re-place via the ladder. A function that + merely *reads* a foreign DTO once to adapt it at a boundary is not envy. diff --git a/go-linter-driven-development/rules/R6-test-only-interfaces.md b/go-linter-driven-development/rules/R6-test-only-interfaces.md index ec79200..2a92fa0 100644 --- a/go-linter-driven-development/rules/R6-test-only-interfaces.md +++ b/go-linter-driven-development/rules/R6-test-only-interfaces.md @@ -75,7 +75,10 @@ deleted. whose only purpose is to break the cycle a double rides in on. - **When an interface is genuinely needed**, define it at the point of use (in the consumer's package), keep it small and cohesive, and expect every implementation - to be production code. + to be production code. The worked case of an *earned* interface β€” multiple + production implementations replacing a growing type switch, sealed by an + unexported method: `../examples/switch-to-polymorphism.md` (dispatch discipline: + `R11-conditional-dispatch.md`). ## Fix pattern diff --git a/go-linter-driven-development/skills/code-designing/SKILL.md b/go-linter-driven-development/skills/code-designing/SKILL.md index ca42d46..70a0631 100644 --- a/go-linter-driven-development/skills/code-designing/SKILL.md +++ b/go-linter-driven-development/skills/code-designing/SKILL.md @@ -3,7 +3,7 @@ name: code-designing description: | FORWARD view over rules/ β€” domain type design and architectural planning for Go code BEFORE it exists. Use when planning new features, designing self-validating types, preventing primitive obsession, or when refactoring reveals need for new types. - Dispatches into the Design guidance sections of rules/R1-R8 and R10. + Dispatches into the Design guidance sections of rules/R1-R8 and R10-R12. allowed-tools: - Skill(go-linter-driven-development:testing) --- @@ -33,6 +33,13 @@ Backward counterpart (fixing code that already fails lint/review): @refactoring. - `function-result-limit` (>3 returns) / `confusing-results` β†’ design a named result type (same R1 scoring) - `file-length-limit` (>450 lines) β†’ split juicy types into their own files (juiciness per R1; file-per-type per `../../rules/R5-vertical-slice.md`); a single god type routes to @refactoring's god-object decomposition procedure first - Package-size yellow/red zone β†’ re-model with sub-packages *before* the zone escalates (@refactoring ``) +- A Phase 4 review CLUSTER (β‰₯2 hunters converging on one anchor β€” + @linter-driven-development routes it here) β†’ **cluster-scoped mode**: skip + `` and the user-OK step (acceptance was inherited when the + findings were accepted); design only the one concept the cluster names β€” its + type or dispatch shape (R11), constructor (R2), mutation surface (R12), and + placement (R4) β€” so every member finding resolves as a consequence of that one + design. Return the mini DESIGN PLAN to the caller; @refactoring implements it. @@ -58,6 +65,27 @@ What is the problem domain? The main concepts/entities? The invariants and rules How does this fit the existing architecture? + +Design happens before a diff exists β€” no detection command can run yet, so questions +are the tool. Interrogate the plan with `../../maxims.md` (the questions live there, +once; ask them, don't restate them): + +- Every function that receives another type's data β†’ **Tell, don't ask**: does the + decision it makes belong on that type? +- Every planned interface β†’ **The bigger the interface, the weaker the abstraction**: + could it be one method? +- Every planned type β†’ **Make illegal states unrepresentable** vs **Make the zero + value useful**: validated domain type (constructor) or mechanism type (useful + zero)? Pick a family. +- Every planned check β†’ **Parse, don't validate**: does it return a more-typed value + or a boolean someone must remember? +- Every shared helper β†’ **A little copying is better than a little dependency**: is + the third strike actually here? + +Answers shape the plan; they are never findings. Maxims propose, evidence disposes β€” +the review phases convict only via rules (`maxims.md`, contract section). + + For each concept in the design, open the rule that owns the question and apply its **Design guidance** section: @@ -73,6 +101,8 @@ For each concept in the design, open the rule that owns the question and apply i | `../../rules/R7-test-placement.md` | The test plan per type: leaf types 100% unit coverage via public constructors; orchestrators integration-tested over real collaborators. | | `../../rules/R8-no-globals.md` | Dependencies injected via constructors, `ctx` threaded from callers, globals only at entry points. | | `../../rules/R10-concurrency-safety.md` | Every planned goroutine gets an owner (stop + wait) and an exit path at construction time; shared state designed with its guard on one type β€” or designed away via handoff/confinement. | +| `../../rules/R11-conditional-dispatch.md` | How each kind/variant family dispatches: behavior-heavy or open set β†’ interface chosen once at the boundary; single-behavior variance β†’ strategy map; single-site closed enum β†’ one exhaustive switch (named enum per R1). | +| `../../rules/R12-mutation-discipline.md` | Each type's mutation surface: constructors copy slice/map arguments; queries return copies or iterators, never internal references; no setters around validating constructors; query and modifier as separate methods. | @@ -87,6 +117,8 @@ Before presenting the plan, verify against the rules (cite, don't restate): - [ ] Import direction strictly downward: leaf types ← sub-packages ← parent ← cmd/ (cycle-breaking move in @refactoring ``) - [ ] Dependencies constructor-injected and validated; ctx flows down; no new globals (R8, R2) - [ ] Every goroutine has an owner and exit path; shared state guarded where it lives, or confined (R10) +- [ ] Every kind/variant family has ONE dispatch owner β€” interface, strategy map, or a single exhaustive switch; no discriminator inspected in two places (R11) +- [ ] Every validated type's mutation surface is closed: slice/map arguments copied in, internal collections never returned by reference, no unvalidated setters (R12) diff --git a/go-linter-driven-development/skills/linter-driven-development/SKILL.md b/go-linter-driven-development/skills/linter-driven-development/SKILL.md index aff442c..f58ecd6 100644 --- a/go-linter-driven-development/skills/linter-driven-development/SKILL.md +++ b/go-linter-driven-development/skills/linter-driven-development/SKILL.md @@ -3,7 +3,7 @@ name: linter-driven-development description: | META ORCHESTRATOR for any Go code change that should end in a commit (features, bug fixes, refactors). WHEN: User requests Go code work (implement, fix, add, refactor), mentions "@ldd"/"ldd", or runs a /go-ldd-* command in a Go project. - Runs the five-phase workflow: DESIGN β†’ IMPLEMENT (per-behavior TDD loop) β†’ FULL LINT (lint-fixer agent) β†’ REVIEW (per slice) β†’ SHIP. + Runs the five-phase workflow (PREPARE is an autonomous sub-phase, 1.5): DESIGN β†’ PREPARE β†’ IMPLEMENT (per-behavior TDD loop) β†’ FULL LINT (lint-fixer agent) β†’ REVIEW (per slice) β†’ SHIP. allowed-tools: - Skill(go-linter-driven-development:code-designing) - Skill(go-linter-driven-development:testing) @@ -14,8 +14,8 @@ allowed-tools: --- -Top-level protocol for Go implementation work: five phases, where Phase 2 is a -per-behavior TDD loop. Rule knowledge lives once in `../../rules/` β€” this skill never +Top-level protocol for Go implementation work: five phases plus the autonomous +PREPARE sub-phase (1.5), where Phase 2 is a per-behavior TDD loop. Rule knowledge lives once in `../../rules/` β€” this skill never restates it; it sequences the thin skills (which dispatch into the rules) and the lint-fixer agent, at the cadence each check's economics demand. @@ -47,8 +47,11 @@ The lint-fixer agent is spawned with the **Task tool**: ``` 1 DESIGN @code-designing β†’ DESIGN PLAN β†’ user OK +1.5 PREPARE survey plan's touch points β†’ four gates decide autonomously β†’ + (@refactoring, preparatory mode) β†’ prep commit(s) Β· PREPARATION LOG (record, no stop) 2 IMPLEMENT β€” per behavior: β”Œβ”€> RED one failing test, lowest rung (@testing) + β”‚ test resists? β†’ prep signal β†’ @refactoring (preparatory) β†’ re-enter RED β”‚ GREEN minimum code to pass β€” no design work β”‚ REFACTOR pkg-scoped lint + rule greps; hits β†’ (@refactoring) └── next behavior until all done @@ -74,6 +77,61 @@ for user OK. **Do not start Phase 2 until the user approves the plan** β€” the R tests target this designed public API, which is how the design reaches GREEN. + +Preparatory refactoring (Fowler: "make the change easy, then make the easy change"): +reshape what the approved plan is about to touch, BEFORE the first RED, so the +feature lands as add-only. Runs **autonomously** β€” the four gates below decide; +this phase never asks the user. + +**Survey**: for each file/package the DESIGN PLAN touches (integration points, +functions it extends, packages receiving new code), run the rule detection greps from +the `../../rules/R*.md` Falsifying questions, scoped to those files only. Same +commands as the REFACTOR step, different premise: this code is probably lint-green +and can still be hostile to the plan. + +**Four gates per finding β€” all mechanical, no user questions:** + +1. **MULTIPLY** β€” would landing the plan add an instance of this violation or force a + workaround (a new case in an already-duplicated switch, R11; new behavior on a raw + primitive, R1; a new step in an at-limit function, R3; new code testable only by + mutating a global, R8)? No β†’ not preparation; leave it for Phase 4's advisory + report. +2. **SAFE** β€” are the paths to reshape covered (`go test -cover` on the touched + packages)? Uncovered β†’ write characterization tests through the public API first + (@testing); they are the move's safety net and keep their value after. When the + missing test seam IS the finding (globals block testing), the prep move creates + the seam β€” R8's Extract Clean Island exists for exactly this. +3. **BOUNDED** β€” effort S/M (hunter scale) β†’ proceed. L β†’ defer to Phase 4's report + as `PREP-DEFERRED`, UNLESS gate 2 showed the feature cannot be tested at all + without it β€” then it is not preparation but a design-plan gap: return to Phase 1. +4. **SKEPTICIZED** β€” any prep move that creates a type/interface/package is judged by + the `overabstraction-skeptic` (Task; payload per @pre-commit-review step 3), with + one sharpening in the spawn prompt: the justification is the approved plan in + hand, not an imagined future β€” score the extraction as if the feature already + existed. REFUTED β†’ apply the cheaper alternative or defer. + +**Apply** the survivors via @refactoring (``); full test suite and +lint green after every move; land the prep work as its own commit(s) before the first +RED β€” the Two Hats at commit granularity, and the reviewer sees reshaping and feature +separately. + +**Emit a PREPARATION LOG** β€” a record, not a question; the loop continues: + +``` +PREPARATION LOG +Touch points surveyed: [files] Β· findings: N +Applied: [rule β†’ move β†’ commit] (gates: multiply βœ“ safe βœ“ bounded βœ“ skeptic βœ“/n-a) +Deferred to Phase 4: [finding β€” failed gate] +Feature landing shape after prep: [add-only / near-add-only / unchanged] +``` + +Zero findings passing the gates is the common case β€” say so in one line and move on. + +Inverse trap: reshaping files the plan does not touch is litter-pickup wearing prep's +clothes β€” a different activity on a different budget; pre-building abstractions this +plan does not need is speculative generality β€” gate 4 exists to kill it. + + One TDD cycle per behavior: @@ -81,6 +139,12 @@ One TDD cycle per behavior: ladder β€” the lowest rung that contains the behavior (@testing, ``). Run it; confirm it fails for the right reason. +If the test *resists* β€” fixture surgery, mutating globals to reach the behavior, +driving three layers to observe one seam β€” do not force it: that friction is a prep +signal Phase 1.5's survey missed. Suspend the cycle, route the friction through the +same four PREPARE gates, apply via @refactoring (``), land the prep +commit, re-enter RED. Autonomous, like Phase 1.5 β€” no user question. + **GREEN** β€” minimum code to pass. Explicitly allowed to be ugly; no design polish in this step. **Never invoke @code-designing from GREEN**: the design already happened in Phase 1 and reaches GREEN through the RED test's shape. If GREEN reveals the @@ -128,6 +192,16 @@ Findings return categorized (Bugs / Design Debt / Readability Debt / Polish), al advisory. Fix bugs and user-accepted findings via @refactoring β€” except accepted R9 (documentation-network) findings, whose fixer is @documentation β€” then re-invoke @pre-commit-review in INCREMENTAL mode until the delta reports clean. + +**Cluster routing**: report entries marked πŸ”— CLUSTER (β‰₯2 hunters converging on one +anchor) are fixed design-first, never member-by-member β€” partial fixes undo each +other (R1 names an enum that R11's move then replaces; R2 places validation that +R11's move relocates). Invoke @code-designing in cluster-scoped mode (it skips the +architecture scan and the user-OK gate β€” acceptance was inherited when the cluster's +findings were accepted; output is a mini DESIGN PLAN for the one concept the cluster +names), then @refactoring implements that plan; the member findings resolve as +consequences of one design. Singleton findings route directly to @refactoring as +before. @@ -141,6 +215,8 @@ advisory. Fix bugs and user-accepted findings via @refactoring β€” except accept - [ ] Design plan user-approved before the first RED +- [ ] PREPARE ran its survey over the plan's touch points; every applied prep move + passed all four gates and landed in its own commit; PREPARATION LOG emitted - [ ] Every behavior completed a RED β†’ GREEN β†’ REFACTOR cycle - [ ] Package-scoped lint + rule greps clean after each cycle - [ ] lint-fixer reported `LINT STATUS: green`; all escalations resolved via @refactoring 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 b059cb1..c7eea67 100644 --- a/go-linter-driven-development/skills/pre-commit-review/SKILL.md +++ b/go-linter-driven-development/skills/pre-commit-review/SKILL.md @@ -13,7 +13,7 @@ allowed-tools: --- -Verify a finished diff against the plugin's rules (R1–R10) with evidence, by orchestrating +Verify a finished diff against the plugin's rules (R1–R12) with evidence, by orchestrating parallel single-obsession `rule-hunter` agents and one `overabstraction-skeptic`. Pure orchestration and reporting: this skill may spawn agents but never edits code, never fixes findings, and never blocks a commit. Rule knowledge lives once in `../../rules/`; @@ -54,6 +54,8 @@ A rule with zero hits is skipped β€” no hunter spawned for it. | R8 | `../../rules/R8-no-globals.md` | package-level state; `context.Background()` in library code | | R9 | `../../rules/R9-repo-brain.md` | orphan docs; broken doc edges (both directions); WHAT-comments on exported API; unwired root | | R10 | `../../rules/R10-concurrency-safety.md` | goroutines without exit paths or owners; unguarded shared-state writes; production sleeps; decorative mutexes | +| R11 | `../../rules/R11-conditional-dispatch.md` | one discriminator switched in β‰₯2 places; type switches in domain logic; unknown-kind defaults away from the boundary; flag arguments; unearned dispatch abstractions (inverse) | +| R12 | `../../rules/R12-mutation-discipline.md` | internal slices/maps returned by reference; constructors aliasing caller collections; query/modifier hybrids; setters around validating constructors; ceremony copies (inverse) | 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 @@ -81,8 +83,8 @@ plus a final tally line (`R: finding(s)` or a hunted-clean line). Collect ALL type/package-extraction findings β€” every R1/R2/R4 "create a type/package" -proposal, and R10 "Extract Synchronized Owner" proposals β€” and spawn one -`overabstraction-skeptic`. Its spawn prompt MUST contain: +proposal, R10 "Extract Synchronized Owner" proposals, and R11 "Interface Dispatch" / +"Strategy Map" proposals β€” and spawn one `overabstraction-skeptic`. Its spawn prompt MUST contain: 1. The extraction findings under review β€” the hunter blocks pasted verbatim. 2. Payload: the **Juiciness scoring** and **The over-abstraction trap** sections of @@ -91,22 +93,51 @@ proposal, and R10 "Extract Synchronized Owner" proposals β€” and spawn one Verdicts per finding: `CONFIRMED (score + verified evidence)` or `REFUTED (score 0–1 + reason) β†’ cheaper alternative`. A refuted proposal does not ship; -when its cheaper alternative (better naming, private fields + accessors) is still worth -doing, report the alternative as 🟒 Polish. Only findings the skeptic cannot kill ship -as extraction findings. Non-extraction findings (R3, R5–R9, and R1/R2/R10 findings -that propose no new type) skip the skeptic and go straight to the report β€” R9 findings -(orphans, broken edges, WHAT-comments, unwired root) propose no type extractions. +when its cheaper alternative (better naming, private fields + accessors, or R11's +Keep the Single Exhaustive Switch) is still worth doing, report the alternative as +🟒 Polish. When R11 dispatch proposals are under review, additionally paste the FULL +content of both R11 case files β€” `../../examples/anti-if-dispatch.md` (Move 3 is the +juiciness rejection: the switch stays, goes exhaustive) and +`../../examples/switch-to-polymorphism.md` (the dependency-direction rejection: the +move is unavailable when the consumer owns the output format; the switch shrinks to +pure dispatch). Only findings the skeptic cannot kill ship as extraction +findings. Non-extraction findings (R3, R5–R9, and R1/R2/R10/R11 findings that propose +no new type) skip the skeptic and go straight to the report β€” R9 findings (orphans, +broken edges, WHAT-comments, unwired root) propose no type extractions. -Merge surviving findings into one report. Category mapping: +Merge surviving findings into one report. + +**Cluster pass (before categorizing):** group surviving findings by shared anchor β€” +the same type, field/discriminator, or function named in β‰₯2 findings from *different* +rules. Each hunter is single-obsession and blind to the others, so independent +convergence on one anchor is evidence that a domain concept is missing there β€” the +cluster is a juiciness scorecard that filled itself in (R1 hunter sees the raw +primitive, R11 the duplicated switch, R2 the ownerless validation: one disease, four +jurisdictions). Render each cluster as a first-class entry above the categories: + +``` +πŸ”— CLUSTER: Alert.Channel (4 findings: R1, R11, R2, R7) + Hypothesis: missing domain concept β€” a Channel type wants to exist + Routing: design-first β€” @code-designing (cluster-scoped), then @refactoring + implements; do NOT fix members independently (partial fixes undo each other) +``` + +Member findings still appear under their categories below, tagged +`[cluster: ]`. Clustering is *reporting* β€” this skill still never edits and +never invokes fix skills; the caller routes. + +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. - πŸ”΄ **Design Debt** β€” R1, R2, R4, R6, R7, R8, R10's non-crash findings (production - sleeps, fire-and-forget ownership, mutex placement), and R5 (advisory β€” never - blocks; the user may have valid reasons): fix before commit recommended. + sleeps, fire-and-forget ownership, mutex placement), R11 (duplicated discriminators, + boundary leaks), R12 (leaked mutable internals, unvalidated setters), and R5 + (advisory β€” never blocks; the user may have valid reasons): fix before commit + recommended. - 🟑 **Readability Debt** β€” R3, R9, unclear naming: improves maintainability. - 🟒 **Polish** β€” minor idiomatic improvements, the skeptic's cheaper alternatives. @@ -119,7 +150,7 @@ Issues noticed outside the diff scope go in a BROADER CONTEXT section, not as fi -**FULL (first run):** pre-filter all ten rules over the whole diff scope; report every +**FULL (first run):** pre-filter all twelve rules over the whole diff scope; report every surviving finding. **INCREMENTAL (re-run after fixes):** diff scope = only files changed since the last diff --git a/go-linter-driven-development/skills/refactoring/SKILL.md b/go-linter-driven-development/skills/refactoring/SKILL.md index 5bd7102..38e3aee 100644 --- a/go-linter-driven-development/skills/refactoring/SKILL.md +++ b/go-linter-driven-development/skills/refactoring/SKILL.md @@ -3,7 +3,8 @@ name: refactoring description: | BACKWARD view over rules/ β€” routes linter and review failures to the rule whose Fix pattern owns the repair. Use when linter fails with complexity issues (cyclomatic, cognitive, maintainability) or when code feels hard to read/maintain. - Applies storifying, type extraction, and function extraction patterns via rules/R1-R8 and R10. + Also runs PREPARATORY mode: reshape code an approved plan touches, before the first RED, so the feature lands add-only. + Applies storifying, type extraction, function extraction, conditional-dispatch, and mutation-discipline patterns via rules/R1-R8 and R10-R12. allowed-tools: - Skill(go-linter-driven-development:code-designing) - Skill(go-linter-driven-development:testing) @@ -41,7 +42,8 @@ table in `../../agents/lint-fixer.md` β€” keep them consistent.) | `funlen` | `../../rules/R3-storifying.md` | | `nestif` | `../../rules/R3-storifying.md` | | `maintidx` | `../../rules/R3-storifying.md` + `../../rules/R1-primitive-obsession.md` | -| `dupl` | `../../rules/R1-primitive-obsession.md` (extract shared type/logic) | +| `dupl` | `../../rules/R1-primitive-obsession.md` (extract shared type/logic); duplicated blocks that switch on the same kind/type discriminator β†’ `../../rules/R11-conditional-dispatch.md` | +| `exhaustive` (missing enum cases) | `../../rules/R11-conditional-dispatch.md` β€” handle the case at the single dispatch site; a second switch appearing is the R11 violation itself | | revive `file-length-limit`; package-size hook failures (`hooks/check-package-sizes.sh`) | `../../rules/R5-vertical-slice.md` β€” mechanics in `` below | | `gochecknoglobals` / `gochecknoinits` | `../../rules/R8-no-globals.md` | | `ireturn` / interface lint on single-impl interfaces | `../../rules/R6-test-only-interfaces.md` | @@ -64,6 +66,8 @@ from there, never from memory: | Move test down a rung, Split `wantErr` tables, Replace sleep with synchronization | `../../rules/R7-test-placement.md` | | Extract Clean Island, Push Global Up One Level, Replace `init()` with constructor, Thread `ctx` | `../../rules/R8-no-globals.md` | | Inject the Exit Path, Make the Goroutine Joinable, Extract Synchronized Owner, Replace Sleep with Timer Select, Delete Unearned Guards | `../../rules/R10-concurrency-safety.md` | +| Replace Duplicated Switch with Interface Dispatch, Replace If-Chain with Strategy Map, Introduce Null Object, Split Flag Argument, Keep the Single Exhaustive Switch | `../../rules/R11-conditional-dispatch.md` | +| Copy on the Way In, Copy on the Way Out / Encapsulate Collection, Separate Query from Modifier, Remove Setting Method, Split Variable | `../../rules/R12-mutation-discipline.md` | **Multi-rule procedures** (sequencing, god-object decomposition, package decomposition): `reference.md` in this directory. @@ -72,6 +76,8 @@ decomposition): `reference.md` in this directory. - Storify β†’ leaf type discovery: `../../examples/storify-leaf-type.md` - Over-abstraction rejection + cheaper alternatives: `../../examples/overabstraction-cidr.md` - Incremental global elimination: `../../examples/dependency-rejection.md` +- Duplicated kind-switch β†’ interface dispatch (and the kept-switch rejection): `../../examples/anti-if-dispatch.md` +- Type switch over an owned interface β†’ fill-style method (and the dependency-direction rejection): `../../examples/switch-to-polymorphism.md` @@ -97,6 +103,35 @@ disease, file count the symptom). Invoke @code-designing to validate extracted t + +Fowler's preparatory refactoring β€” "make the change easy, then make the easy change": +reshape code an approved plan is about to touch, before the first RED, so the feature +lands as add-only. Invoked by @linter-driven-development (Phase 1.5, or Phase 2 RED +friction) or `/go-ldd-prepare`, with a DESIGN PLAN, the touch-point file list, and +findings that already passed the four PREPARE gates (multiply / safe / bounded / +skeptic β€” the gates live in @linter-driven-development ``; this +mode trusts their verdicts and re-runs none of them). Fully autonomous β€” no user +confirmation, same as the rest of this skill. + +Differences from failure-driven operation: + +- **The trigger is the plan, not the linter.** Targets are usually lint-green; + "still failing β†’ next move" does not apply. Route each finding by its rule (the + same `` rules own the same fix patterns) and apply. +- **Safety before motion.** Uncovered paths get characterization tests through the + public API first (@testing); the full suite β€” not just the touched package β€” runs + green after every move, because prep edits existing behavior by definition. +- **Stopping criterion β€” landing shape, not lint.** Stop when the planned change + lands as add-only or near-add-only: a new variant = one new file plus one case at + the dispatch boundary (R11); new behavior = a method on an existing type (R1); new + code = testable without touching globals (R8). Re-check against the plan after + each move; shape reached β†’ STOP, even with findings left β€” those were never + preparation and belong to Phase 4's advisory report. +- **Commits are segregated.** Prep work lands in its own commit(s), never mixed with + feature code β€” the reviewer sees behavior-preserving reshaping and new behavior as + separate diffs. + + 1. Receive trigger (from @linter-driven-development, from the caller acting on accepted @pre-commit-review findings, or manual). @@ -106,7 +141,10 @@ disease, file count the symptom). Invoke @code-designing to validate extracted t 4. Still failing β†’ next move in the sequence. Repeat until green. 5. **Escalation**: complexity failures that keep recurring mean a new type or design is needed β€” invoke @code-designing. Patterns exhausted β†’ report what was tried and - escalate to the user for architectural guidance. + escalate to the user for architectural guidance. Frame the escalation in maxim + vocabulary (`../../maxims.md`) β€” name *why* the code resists ("every caller asks + this struct three questions and then decides β€” the design wants Tell-Don't-Ask"), + not just which linter stayed red. @@ -154,7 +192,8 @@ STATUS: [linter green / still failing: N issues / escalated to @code-designing] -**Invoked by**: @linter-driven-development (Phase 3, lint failures), or the caller acting +**Invoked by**: @linter-driven-development (Phase 1.5 / RED friction β†’ ``; +Phase 3, lint failures), or the caller acting on accepted @pre-commit-review findings (@linter-driven-development Phase 4 accepted findings, or the user) β€” @pre-commit-review reports only and never invokes fix skills. **Invokes**: @code-designing (new types/design needed), @testing (after every extraction diff --git a/ts-react-linter-driven-development/.claude-plugin/plugin.json b/ts-react-linter-driven-development/.claude-plugin/plugin.json index c9a09ac..4b7ea00 100644 --- a/ts-react-linter-driven-development/.claude-plugin/plugin.json +++ b/ts-react-linter-driven-development/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ts-react-linter-driven-development", - "version": "1.0.0", - "description": "Linter-driven development workflow for TypeScript + React with six specialized skills: component design, testing, refactoring, review, and documentation", + "version": "1.2.0", + "description": "Linter-driven development workflow for TypeScript + React with six specialized skills: meta orchestrator, component design, testing, refactoring, review, and documentation", "author": { "name": "Dan Mordechay" }, diff --git a/ts-react-linter-driven-development/skills/component-designing/SKILL.md b/ts-react-linter-driven-development/skills/component-designing/SKILL.md index ce62e58..f438509 100644 --- a/ts-react-linter-driven-development/skills/component-designing/SKILL.md +++ b/ts-react-linter-driven-development/skills/component-designing/SKILL.md @@ -512,6 +512,7 @@ See reference.md for detailed principles: - Custom hooks for reusable logic - Context for shared state (use sparingly) - Props interfaces and type safety +- One dispatch owner per variant family (Anti-IF): model variants as discriminated unions; render them via component/lookup maps, never the same kind-conditional in multiple components ## Pre-Code Review Questions @@ -521,6 +522,7 @@ Before writing code, ask: - Should state be local or context? - Have I avoided primitive obsession? - Is validation in the right place? +- Does each variant family (kind/status/type) have exactly one dispatch owner β€” a discriminated union rendered through a component map or one exhaustive switch? - Does this follow the existing codebase architecture? - Are components small and focused? diff --git a/ts-react-linter-driven-development/skills/refactoring/SKILL.md b/ts-react-linter-driven-development/skills/refactoring/SKILL.md index 8641d91..b032314 100644 --- a/ts-react-linter-driven-development/skills/refactoring/SKILL.md +++ b/ts-react-linter-driven-development/skills/refactoring/SKILL.md @@ -529,6 +529,149 @@ function LoginForm() { } ``` +### Pattern 8: Replace Tag-Based Conditionals with Dispatch (Anti-IF) + +**Signal**: The same discriminator (`kind`, `status`, `type` field) inspected in more than one place; ternary/if-else chains in JSX selecting between renderings; switch statements duplicated across files + +The Anti-IF principle: a conditional that asks what a value *is* may exist once. The second copy of the same `switch (x.kind)` is a missing abstraction β€” dispatch through a lookup map or let a discriminated union carry the decision. + +```typescript +// ❌ Before - Same discriminator inspected in three places +function StatusBadge({ status }: { status: Status }) { + return status === 'active' ? ( + Active + ) : status === 'pending' ? ( + Pending + ) : ( + Suspended + ) +} + +function statusLabel(status: Status) { + if (status === 'active') return 'Active' + if (status === 'pending') return 'Pending approval' + return 'Suspended' // drifts independently of StatusBadge +} + +function canLogin(status: Status) { + if (status === 'active') return true + if (status === 'pending') return true + return false +} + +// βœ… After - One dispatch owner: a config map keyed by the discriminator +const STATUS_CONFIG: Record = { + active: { color: 'green', label: 'Active', canLogin: true }, + pending: { color: 'yellow', label: 'Pending approval', canLogin: true }, + suspended: { color: 'red', label: 'Suspended', canLogin: false } +} + +function StatusBadge({ status }: { status: Status }) { + const { color, label } = STATUS_CONFIG[status] + return {label} +} + +// Adding a status = adding one entry; TypeScript errors until every +// Record key is present β€” the compiler enforces completeness. +``` + +For variant components, dispatch through a component map instead of ternary chains in JSX: + +```typescript +// ❌ Before - Nested ternaries selecting components +function NotificationItem({ notification }: { notification: Notification }) { + return notification.kind === 'message' ? ( + + ) : notification.kind === 'mention' ? ( + + ) : ( + + ) +} + +// βœ… After - Component map, one entry per variant. +// `satisfies` a mapped type: each component is checked against ITS OWN variant +// (MessageNotification must accept Extract), +// and a missing kind is a compile error β€” don't widen every view to the union, +// which would force each component to re-narrow internally (re-asking). +const NOTIFICATION_VIEWS = { + message: MessageNotification, + mention: MentionNotification, + system: SystemNotification +} satisfies { + [K in Notification['kind']]: FC<{ data: Extract }> +} + +function NotificationItem({ notification }: { notification: Notification }) { + // TypeScript cannot correlate the map entry with the narrowed union here + // (correlated-union limitation), so ONE localized cast sits at the single + // dispatch site β€” made safe in practice by the `satisfies` check above. + const View = NOTIFICATION_VIEWS[notification.kind] as FC<{ data: Notification }> + return +} +``` + +When a switch legitimately stays (single site, variant-specific payloads), make it a **discriminated union with an exhaustiveness check** β€” the compiler then does what an if-chain never could: fail the build when a variant is added but not handled: + +```typescript +type PaymentEvent = + | { kind: 'charge'; amountCents: number } + | { kind: 'refund'; amountCents: number; reason: string } + | { kind: 'dispute'; caseId: string } + +function describe(event: PaymentEvent): string { + switch (event.kind) { + case 'charge': + return `Charged ${formatCents(event.amountCents)}` + case 'refund': + return `Refunded ${formatCents(event.amountCents)}: ${event.reason}` + case 'dispute': + return `Dispute opened (${event.caseId})` + default: { + const unhandled: never = event // compile error if a variant is missed + return unhandled + } + } +} +``` + +**The dividing line**: dispatch is bought with the deletion of duplicated decisions. One switch at one site over a discriminated union is healthy β€” keep it exhaustive. The violation is the *second* copy of the discriminator, or `boolean` props that select between behaviors (`