/solid

name: solid
description: >
  SOLID design review rules (SRP, OCP, LSP, ISP, DIP) for code reviews: what counts as a real
  design finding, what is only taste, how to cite it and how severe it is. Use when reviewing a
  diff, a pull request or recent changes for design problems, when asked for a "SOLID review",
  "design review", "is this class doing too much", or when a review checklist asks for
  SOLID. Pair it with the dry skill for duplication and with owasp-asvs for security.

SOLID Review

A design finding is worth reporting only when the design **already costs something or will on the next ordinary change** — a bug, an inconsistency, a change that has to touch many places, a unit that cannot be tested. Style, naming taste, "could be more elegant" and textbook purity are never findings.

Scope

Review the changed code and what it calls, read whole — not only the diff hunks. Judge the change against the conventions the repository already follows (its `AGENTS.md`/`CLAUDE.md`, existing seams and patterns). A repo that keeps small scripts procedural is not violating SOLID by doing so.

Rules

SRP — Single Responsibility

One unit (function, class, module, service) has to change for **two unrelated reasons**. Name both reasons in the finding.

• Finding: an HTTP controller that validates input, builds SQL, renders e-mail and spawns a

child process; a Drupal form submit handler that also syncs to a remote API.

• Not a finding: a long function that does one thing in several steps.

OCP — Open/Closed

Adding the **next variant** (a check, a backend, a provider, a content type) requires editing a `switch`/`if` chain in **several** places instead of one registration point (a map, a plugin type, a tagged service, an event subscriber).

• Cite every place the next variant would have to touch.
• Not a finding: a single `match` in one place that is the registration point.

LSP — Liskov Substitution

An implementation that **throws, no-ops or silently changes meaning** for part of the contract it claims, so that callers must know which concrete class they have (`instanceof` checks, "not supported" exceptions, a subclass that ignores an argument).

ISP — Interface Segregation

Callers are forced to depend on methods they never use, or implementers must stub out half an interface. Cite the unused methods and the stubbing implementers.

DIP — Dependency Inversion

Business logic hard-wired to a concrete I/O dependency (network, filesystem, database, clock, child process, a static `\Drupal::service()` call inside a class that supports injection) so that it cannot be tested or swapped without it — **where the codebase already has a seam** for that dependency elsewhere (DI container, an injected client, a clock service). Without an existing seam, report it only if it already blocks a test the change needs.

Severity

• `high` — the design problem has **already produced** a bug or an inconsistency you can point

to (two branches of an `if` chain disagree, a stubbed method is called in production).

• `medium` — default: the next ordinary change will have to touch several places or cannot be

tested without real I/O.

• `low` — cleanup that would help but costs nothing today.

Reporting a finding

Each finding carries:

• **rule** — `SRP`, `OCP`, `LSP`, `ISP` or `DIP`;
• **location** — `file:line` (every location for OCP);
• **cost** — one sentence: what breaks or what the next change has to do because of it;
• **fix direction** — the smallest change that removes the cost, preferring a seam the codebase

already has over a new abstraction.

Verify each finding against the code before keeping it: re-read the lines and confirm the cost is real. Fewer, true findings beat a long list. A disagreement with the overall approach of a change is one finding, not a rewrite.