Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. `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.6

- 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: <id>)*` 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
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions agents/go-architecture-assistant.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
2 changes: 2 additions & 0 deletions agents/node-quality-assistant.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
</role>

Expand Down
2 changes: 2 additions & 0 deletions agents/python-architecture-assistant.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
5 changes: 4 additions & 1 deletion docs/architecture-dimensions-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
66 changes: 66 additions & 0 deletions docs/code-smell-vocabulary.md
Original file line number Diff line number Diff line change
@@ -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
9 changes: 9 additions & 0 deletions docs/go-architecture-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions docs/node-service-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions docs/python-architecture-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions llms.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading