From bf7fdff502be1064937ed9cdf05769799452a474 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 23:40:49 +0200 Subject: [PATCH 1/2] add shared code smell vocabulary doc --- CHANGELOG.md | 4 ++ README.md | 1 + docs/architecture-dimensions-guide.md | 5 +- docs/code-smell-vocabulary.md | 66 +++++++++++++++++++++++++++ docs/go-architecture-patterns.md | 9 ++++ docs/node-service-guide.md | 12 +++++ docs/python-architecture-patterns.md | 12 +++++ llms.txt | 1 + 8 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 docs/code-smell-vocabulary.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 88637be..8e22348 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- docs: add `code-smell-vocabulary.md` — Fowler's 12 structural code smells as one-line definitions in a single language-agnostic doc, replacing the alternative of restating them per language. Only 1 of the 12 ("shotgun surgery") existed anywhere in the repo before this. The doc splits mechanical-tier smells (long method, large class, long parameter list, duplicated code, dead code — already owned by `funlen` / `dupl` / `vulture` / `ts-prune`) from judgment-tier ones, so review agents stop re-reporting what a linter already catches. `go-architecture-patterns.md`, `python-architecture-patterns.md`, and `node-service-guide.md` now link the shared definitions and carry only the language-specific fix (Go named types vs Python `NewType` vs TS branded types); Node had no structural-smell vocabulary at all before. `architecture-dimensions-guide.md` §7 names divergent change and speculative generality as evolvability probes — the latter was already described there unnamed — and states explicitly that the other nine smells are structural and out of scope for the behavioral pass, per its own scope table + ## v0.35.5 - fix: pr-review — every finding must now lead with a bold file reference (``**`path:LINE`**``, ``**`path`**`` or ``**`path:START-END`**``) and carry the inline `*(rule: )*` tag when it maps to a rule, so a finding is always attributable to a location. The first bold run must *be* the reference, not a summary phrase; a path appearing only in the item's prose does not count and no path may be inferred that was not in the diff. General remarks belong in a trailing `**Notes:**` block, never as a bullet inside a severity section. Measured before the change: 6 of 15 benchmark review attempts (40%) were rejected as unattributable, non-deterministically — after it, 10 of 10 findings across two runs carry a path and no attempt was rejected diff --git a/README.md b/README.md index 6d4b6db..8f9a975 100644 --- a/README.md +++ b/README.md @@ -188,6 +188,7 @@ All guides live in [`docs/`](docs/) and can be read standalone without the plugi | [PRD Guide](docs/prd-guide.md) | Product Requirements | | [ADR Guide](docs/adr-guide.md) | Architecture Decisions | | [Architecture Dimensions](docs/architecture-dimensions-guide.md) | Whole-codebase behavioral review — 8 dimensions (data flow, failure, concurrency, observability, drift) | +| [Code Smell Vocabulary](docs/code-smell-vocabulary.md) | Fowler's 12 structural smells, language-agnostic — shared by the Go/Python/Node review passes | | [Markdown & Todos](docs/markdown-todo-guide.md) | Formatting standards | ### Claude Code Authoring diff --git a/docs/architecture-dimensions-guide.md b/docs/architecture-dimensions-guide.md index 3dc6a1c..f924e93 100644 --- a/docs/architecture-dimensions-guide.md +++ b/docs/architecture-dimensions-guide.md @@ -140,10 +140,13 @@ Pick one plausible next feature. Count files / packages you'd touch. **Check:** - Shotgun surgery → abstraction is at the wrong altitude. -- Touching one file → boundary is right OR abstraction is premature (does the feature actually need swap-in/swap-out?). +- Divergent change — the same file keeps changing for unrelated reasons → one module is carrying several jobs. +- Touching one file → boundary is right OR speculative generality (does the feature actually need swap-in/swap-out?). - Dead-code dispatch — config field exists but runtime always picks one path? (Silent misconfiguration.) - Asymmetric abstractions — one provider has its own package, another is hardcoded in a "generic" package? +These three terms are defined in [code-smell-vocabulary.md](code-smell-vocabulary.md); here they act as evolvability probes, not as a structural checklist. The vocabulary's other nine smells are structural and belong to the sibling assistants per the scope table above — do not report them from this pass. + **Severity:** - **Critical** if a configurable feature is dead code at runtime (silent misconfig). diff --git a/docs/code-smell-vocabulary.md b/docs/code-smell-vocabulary.md new file mode 100644 index 0000000..c9f15c7 --- /dev/null +++ b/docs/code-smell-vocabulary.md @@ -0,0 +1,66 @@ +# Code Smell Vocabulary + +Twelve named code smells from Martin Fowler's *Refactoring*, as one-line definitions. Language-agnostic — Go, Python, Node, and any other language reviewed by this plugin share these definitions. + +## Why a vocabulary instead of instructions + +A named smell is denser than the paragraph that describes it. "Check for shotgun surgery" and "check whether changing one requirement would force edits in several files, which risks missing one and creating inconsistency" instruct the same check — but the first is a term with an established meaning, and the second is prose a reviewer has to parse. + +For review agents this matters twice over: extra words are distractions that invite hallucination, and a shared term means a finding is reviewable ("that's feature envy") instead of arguable. + +**Use the term. Do not restate the definition inline.** If a review report needs to explain a smell, link here. + +## The twelve + +| Term | Definition | +|---|---| +| **Shotgun surgery** | One change forces edits in many places; miss one and behavior goes inconsistent. | +| **Divergent change** | One module changes for many unrelated reasons — the inverse of shotgun surgery. | +| **Feature envy** | Logic lives in a module other than the one owning the data it works on. | +| **Data clumps** | The same group of values travels together through many signatures without being a type. | +| **Long method** | A unit doing too much to hold in one reading. | +| **Large class** | A type accreting responsibilities that change for unrelated reasons. | +| **Long parameter list** | Callers must assemble many positional arguments, inviting silent order drift. | +| **Primitive obsession** | Domain concepts encoded as bare strings, ints, or maps instead of named types. | +| **Duplicated code** | The same logic maintained in more than one place. | +| **Message chains** | A caller navigates `a.b().c().d()`, coupling it to structure it should not know. | +| **Dead code** | Nothing depends on it; deleting it changes no observable behavior. | +| **Speculative generality** | An abstraction built for a use case that never arrived. | + +## Which layer catches which + +Naming all twelve does not mean all twelve are worth an agent's judgment — several are already caught mechanically, and re-checking them by hand produces noise. + +| Layer | Smells | Why | +|---|---|---| +| **Mechanical** (linters: `funlen`, `dupl`, `gocyclo`, `vulture`, `ts-prune`) | Long method, Large class, Long parameter list, Duplicated code, Dead code | Threshold- or reachability-detectable. Cheaper and more consistent than judgment. | +| **Judgment** (architecture assistants) | Shotgun surgery, Divergent change, Feature envy, Primitive obsession, Message chains, Data clumps, Speculative generality | Require knowing what the code *means* — no threshold separates a good abstraction from a speculative one. | + +A judgment-tier finding still needs a `file:line` citation and a named structural fix, not "consider refactoring". + +## Structural vs behavioral ownership + +These smells are **structural** — they describe how code is organized. They belong to the structural review pass: + +- `go-architecture-assistant` — see [go-architecture-patterns.md](go-architecture-patterns.md) +- `python-architecture-assistant` — see [python-architecture-patterns.md](python-architecture-patterns.md) +- `node-quality-assistant` — see [node-service-guide.md](node-service-guide.md) + +The **behavioral** pass ([architecture-dimensions-guide.md](architecture-dimensions-guide.md)) covers data flow, failure paths, concurrency, observability, and drift. Three of these terms also work as evolvability probes there — shotgun surgery, divergent change, and speculative generality all answer "what would the next feature cost?" — and §7 of that guide uses them in that role. That is the only overlap; the other nine stay structural. + +## Antipatterns + +| ❌ | ✅ | +|---|---| +| Copying these definitions into a language-specific guide | Linking here; language guides carry only the language-specific fix | +| Inventing house terms ("widget drift") | Using established vocabulary the model already knows | +| Reporting a mechanical-tier smell as an architectural finding | Letting the linter own it; report only judgment-tier smells | +| "This has feature envy" with no location | `pkg/order/service.go:88 — feature envy: inventory math on an order type` | +| Naming a smell as the whole finding | Naming the smell, the site, and the structural fix | + +## Related + +- [architecture-dimensions-guide.md](architecture-dimensions-guide.md) — behavioral pass; §7 uses three of these as evolvability probes +- [go-architecture-patterns.md](go-architecture-patterns.md) — Go structural patterns +- [python-architecture-patterns.md](python-architecture-patterns.md) — Python structural patterns +- [node-service-guide.md](node-service-guide.md) — Node service structure diff --git a/docs/go-architecture-patterns.md b/docs/go-architecture-patterns.md index 2d29151..4eb752a 100644 --- a/docs/go-architecture-patterns.md +++ b/docs/go-architecture-patterns.md @@ -712,6 +712,15 @@ service-name/ ## 11. Common Antipatterns to Avoid +Structural smells are named once, language-agnostically, in [code-smell-vocabulary.md](code-smell-vocabulary.md) — use those terms in review findings rather than restating them. The Go-specific fixes below are the idiom layer on top of that vocabulary: + +| Smell | Go fix | +|---|---| +| Primitive obsession | Named types (`type UserID string`) over bare `string` / `int`; see § 8 Core Types | +| Data clumps | A struct with a `New*` constructor over repeated positional parameters | +| Long parameter list | Functional options (`go-functional-options-pattern.md`) or a config struct | +| Feature envy | Move the method to the type owning the data; see § 7 Dependency Injection | + ### DON'T: Create custom pointer helper functions ```go // DON'T DO THIS diff --git a/docs/node-service-guide.md b/docs/node-service-guide.md index 954163b..59ddec6 100644 --- a/docs/node-service-guide.md +++ b/docs/node-service-guide.md @@ -517,8 +517,20 @@ spec: New services are TypeScript; the rules above apply unchanged to `.ts` sources. Node runs `.ts` directly by stripping type annotations, so no build step is required — but stripping does **not** type-check, so `tsc --noEmit` must run in `make check` or the annotations are decoration. Prefer erasable syntax (string-literal unions over `enum`, explicit field assignment over constructor parameter properties) so the no-build-step property holds. +## Structural Smells + +Named once, language-agnostically, in `code-smell-vocabulary.md` — use those terms in review findings rather than restating them. TypeScript/Node-specific fixes: + +| Smell | Node fix | +|---|---| +| Primitive obsession | Branded types (`type UserId = string & {readonly _brand: unique symbol}`) or a Zod schema over bare `string` | +| Data clumps | An interface or Zod object over repeated positional arguments | +| Long parameter list | A single options object — the idiomatic JS form | +| Feature envy | Move the function to the module owning the data; see § Dependency Injection | + ## Related +- `code-smell-vocabulary.md` — the twelve structural smells, shared across languages - `node-makefile-commands.md` — build, test, and check targets - `k8s-manifest-guide.md` — generic manifest conventions - `go-http-service-guide.md` — the equivalent contract in Go diff --git a/docs/python-architecture-patterns.md b/docs/python-architecture-patterns.md index 7677cc6..c136f97 100644 --- a/docs/python-architecture-patterns.md +++ b/docs/python-architecture-patterns.md @@ -558,8 +558,20 @@ def test_user_service_integration(): assert fake_repo.find_by_id(1).name == "Alice" ``` +## Structural Smells + +Named once, language-agnostically, in [code-smell-vocabulary.md](code-smell-vocabulary.md) — use those terms in review findings rather than restating them. Python-specific fixes: + +| Smell | Python fix | +|---|---| +| Primitive obsession | `NewType("UserID", str)` or a Pydantic model over bare `str` / `dict` | +| Data clumps | A `@dataclass` / Pydantic model over repeated keyword arguments | +| Long parameter list | A config dataclass, or keyword-only arguments with defaults | +| Feature envy | Move the method to the class owning the data; see § 1 Constructor Injection | + ## Related Documentation +- [code-smell-vocabulary.md](code-smell-vocabulary.md) - The twelve structural smells, shared across languages - [python-project-structure.md](python-project-structure.md) - Project layout, pyproject.toml, src/ layout, test organization - [python-factory-pattern.md](python-factory-pattern.md) - Detailed factory patterns, antipatterns, file organization - [python-ioc-guide.md](python-ioc-guide.md) - Detailed DI patterns, Protocol vs ABC, async patterns diff --git a/llms.txt b/llms.txt index 408d45d..9c2c2a4 100644 --- a/llms.txt +++ b/llms.txt @@ -90,6 +90,7 @@ - [PRD Guide](docs/prd-guide.md): Product Requirements Documents - [ADR Guide](docs/adr-guide.md): Architecture Decision Records - [Architecture Dimensions](docs/architecture-dimensions-guide.md): Whole-codebase behavioral review — 8 dimensions (data flow, failure, concurrency, observability, drift, blast radius, evolvability, cross-cutting consistency); paired with `architecture-dimensions-assistant` agent and `/coding:architecture-review` command +- [Code Smell Vocabulary](docs/code-smell-vocabulary.md): Fowler's 12 structural code smells as one-line definitions, language-agnostic; shared by `go-architecture-assistant`, `python-architecture-assistant`, `node-quality-assistant`; splits mechanical-tier (linter-owned) from judgment-tier smells - [Markdown & Todos](docs/markdown-todo-guide.md): Formatting standards ## Acceptance Scenarios From 97c6641a03ca01779471c062bd585df633d3445a Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 23:46:37 +0200 Subject: [PATCH 2/2] load code smell vocabulary from the three architecture agents --- CHANGELOG.md | 2 +- agents/go-architecture-assistant.md | 2 ++ agents/node-quality-assistant.md | 2 ++ agents/python-architecture-assistant.md | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e22348..24f9ace 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## Unreleased -- docs: add `code-smell-vocabulary.md` — Fowler's 12 structural code smells as one-line definitions in a single language-agnostic doc, replacing the alternative of restating them per language. Only 1 of the 12 ("shotgun surgery") existed anywhere in the repo before this. The doc splits mechanical-tier smells (long method, large class, long parameter list, duplicated code, dead code — already owned by `funlen` / `dupl` / `vulture` / `ts-prune`) from judgment-tier ones, so review agents stop re-reporting what a linter already catches. `go-architecture-patterns.md`, `python-architecture-patterns.md`, and `node-service-guide.md` now link the shared definitions and carry only the language-specific fix (Go named types vs Python `NewType` vs TS branded types); Node had no structural-smell vocabulary at all before. `architecture-dimensions-guide.md` §7 names divergent change and speculative generality as evolvability probes — the latter was already described there unnamed — and states explicitly that the other nine smells are structural and out of scope for the behavioral pass, per its own scope table +- docs: add `code-smell-vocabulary.md` — Fowler's 12 structural code smells as one-line definitions in a single language-agnostic doc, replacing the alternative of restating them per language. Only 1 of the 12 ("shotgun surgery") existed anywhere in the repo before this. The doc splits mechanical-tier smells (long method, large class, long parameter list, duplicated code, dead code — already owned by `funlen` / `dupl` / `vulture` / `ts-prune`) from judgment-tier ones, so review agents stop re-reporting what a linter already catches. `go-architecture-patterns.md`, `python-architecture-patterns.md`, and `node-service-guide.md` now link the shared definitions and carry only the language-specific fix (Go named types vs Python `NewType` vs TS branded types); Node had no structural-smell vocabulary at all before. `architecture-dimensions-guide.md` §7 names divergent change and speculative generality as evolvability probes — the latter was already described there unnamed — and states explicitly that the other nine smells are structural and out of scope for the behavioral pass, per its own scope table. `go-architecture-assistant`, `python-architecture-assistant`, and `node-quality-assistant` each name the shared doc in their source-of-truth block, so the vocabulary is loaded rather than reachable only by following a link from a companion guide ## v0.35.5 diff --git a/agents/go-architecture-assistant.md b/agents/go-architecture-assistant.md index 6157af6..b1f4ca9 100644 --- a/agents/go-architecture-assistant.md +++ b/agents/go-architecture-assistant.md @@ -14,6 +14,8 @@ You are a Go architecture reviewer. Your job is to distinguish **real design imp **Source of truth (rule definitions):** `rules/index.json` entries with `owner: go-architecture-assistant`. Companion guides (`go-architecture-patterns.md`, `go-composition.md`, `go-service-implementation-patterns.md`, `go-state-machine-pattern.md`, `go-kubernetes-crd-controller-guide.md`, `go-concurrency-patterns.md`, `go-enum-type-pattern.md`, `go-cqrs.md`, `k8s-manifest-guide.md`, `go-filter-pattern.md`, `go-boolean-combinator-pattern.md`, `adr-guide.md`) carry the `### RULE` blocks; consult for context. +**Shared vocabulary:** `docs/code-smell-vocabulary.md` — name structural findings with those twelve terms rather than describing the smell in prose. Report only its judgment tier; the mechanical tier (long method, large class, long parameter list, duplicated code, dead code) is owned by `funlen` / `dupl` / linters and must not be re-reported here. + ## When invoked by the dispatcher The dispatcher calls this agent with pre-filtered mechanical findings + judgment-tier rule IDs you own. Adjudicate severity, cite the rule by ID. Don't re-scan for mechanical violations. Every emitted `rule_id` MUST exist in `rules/index.json`. diff --git a/agents/node-quality-assistant.md b/agents/node-quality-assistant.md index d565aa5..0b1d9c3 100644 --- a/agents/node-quality-assistant.md +++ b/agents/node-quality-assistant.md @@ -17,6 +17,8 @@ Senior Node.js engineer performing targeted code quality review of backend servi Source of truth (rule definitions): `rules/index.json` entries with `owner: node-quality-assistant`. Companion guides: `node-service-guide.md`, `node-makefile-commands.md`, `k8s-manifest-guide.md`. +Shared vocabulary: `docs/code-smell-vocabulary.md` — name structural findings with those twelve terms rather than describing the smell in prose. Report only its judgment tier; the mechanical tier (long method, large class, long parameter list, duplicated code, dead code) is owned by ESLint / `ts-prune` and must not be re-reported here. + Rules live in the docs, not in this file. Read `docs/node-service-guide.md` before evaluating; never re-state a rule from memory. diff --git a/agents/python-architecture-assistant.md b/agents/python-architecture-assistant.md index ee5f6f4..c9ccb63 100644 --- a/agents/python-architecture-assistant.md +++ b/agents/python-architecture-assistant.md @@ -14,6 +14,8 @@ You are a Python architecture reviewer. Adjudicate findings the `ast-grep-runner **Source of truth (rule definitions):** `rules/index.json` entries with `owner: python-architecture-assistant`. Companion guides: `python-architecture-patterns.md`, `python-ioc-guide.md`, `python-project-structure.md`. +**Shared vocabulary:** `docs/code-smell-vocabulary.md` — name structural findings with those twelve terms rather than describing the smell in prose. Report only its judgment tier; the mechanical tier (long method, large class, long parameter list, duplicated code, dead code) is owned by `mccabe` / `pylint` / `vulture` and must not be re-reported here. + ## When invoked by the dispatcher Dispatcher calls this agent with pre-filtered mechanical findings + judgment-tier rule IDs you own. Adjudicate severity, cite the rule by ID. Don't re-scan for mechanical violations. Every emitted `rule_id` MUST exist in `rules/index.json`.