feat(conventions): modularity checks + scan-derived, language-aware rules

The standard was architectural LINTING (placement + hygiene) — things ruff/eslint already do — and it forced backend rules onto frontend projects. This makes it enforce MODULARIZATION, the separation-of-concerns a senior demands that linters are blind to:

- modular_cohesion: a file that mixes architectural concerns (a model defined in a router, a schema in a component) is a monolith — split it. One concern per file.
- thin_routes (Python): a route handler that runs its own DB access instead of delegating to a service.
- thin_components (TypeScript/React): a component that fetches data in its body instead of using a hook.
- god_class: a class past a method-count threshold (single responsibility).

The checks inspect a definition's BODY and a file's COMPOSITION via tree-sitter, precision-over-recall (fire only on a confident structural signal). Rules are now scan-derived and language-aware: hygiene seeds universally, placement only for modules that exist, and modularity per stack — so a frontend project carries no_models_in_components + thin_components, never a backend no_models_in_routers. BUILTIN_RULES is reduced to language-agnostic hygiene.
This commit is contained in:
Renn F
2026-06-22 14:26:25 +02:00
parent 63b6114364
commit 4a8c508f91
7 changed files with 533 additions and 14 deletions
+342
View File
@@ -0,0 +1,342 @@
"""Modularity checks — separation of concerns the linters cannot see.
These are the senior-review judgements that ruff / eslint / mypy are blind to:
a file that mixes architectural concerns (a model defined inside a router), a
route handler that does its own data access instead of delegating to a service,
a React component that fetches data instead of using a hook, a class that has
grown into a god object. They inspect a file's COMPOSITION and a definition's
BODY, not just its top-level kind — which is what makes them about *quality*,
not lint.
Precision over recall: every check fires only on a confident, structural signal,
so a ``block``-level gate is never tripped by a guess. The rule names line up
with the per-project rule set so each can be levelled (warn/block) or waived.
"""
from __future__ import annotations
from typing import TYPE_CHECKING
from .findings import Finding
from .grammars import get_parser
if TYPE_CHECKING:
from tree_sitter import Node
from roboco.foundation.policy.conventions.models import ConventionsStandard
from .placement import Definition
# The architectural concerns whose co-location in one file is a modularity smell.
# A file should own a single one of these.
_CORE_KINDS = frozenset({"model", "route", "component"})
# A file may own at most this many distinct architectural concerns before it is
# a monolith that should be split.
_MAX_CONCERNS_PER_FILE = 1
# SQLAlchemy session methods + 2.0 constructs that signal data access. A route
# whose body calls one of these is doing a repository's / service's job.
_DB_METHODS = frozenset(
{
"execute",
"scalar",
"scalars",
"commit",
"add",
"add_all",
"flush",
"refresh",
"merge",
"query",
}
)
_DB_CONSTRUCTS = frozenset({"select", "insert", "update", "delete"})
# Data-fetching that belongs in a hook / query layer, not in a component body.
_FETCH_IDENTIFIERS = frozenset({"fetch", "axios"})
# A class with more methods than this is doing too much (single responsibility).
_GOD_CLASS_METHODS = 15
_ROUTER_OBJECTS = frozenset({"router", "app"})
_HTTP_METHODS = frozenset({"get", "post", "put", "delete", "patch"})
_JSX_TYPES = frozenset({"jsx_element", "jsx_self_closing_element", "jsx_fragment"})
def check_modularity(
rel_path: str,
defs: list[Definition],
source: bytes,
language: str,
standard: ConventionsStandard,
) -> list[Finding]:
"""Return modularity findings for one changed file."""
findings = _check_cohesion(rel_path, defs, standard)
if language == "python":
findings += _check_python_bodies(rel_path, source, standard)
else:
findings += _check_ts_bodies(rel_path, source, language, standard)
return findings
def _level(standard: ConventionsStandard, rule: str, default: str) -> str:
rule_obj = standard.rules.get(rule)
return rule_obj.level if rule_obj is not None else default
# --------------------------------------------------------------------------- #
# Cohesion — one architectural concern per file
# --------------------------------------------------------------------------- #
def _check_cohesion(
rel_path: str, defs: list[Definition], standard: ConventionsStandard
) -> list[Finding]:
core = [(name, line, kind) for name, line, kind in defs if kind in _CORE_KINDS]
kinds = {kind for _name, _line, kind in core}
if len(kinds) <= _MAX_CONCERNS_PER_FILE:
return []
first_line = min(line for _name, line, _kind in core)
return [
Finding(
file=rel_path,
line=first_line,
kind=None,
rule="modular_cohesion",
level=_level(standard, "modular_cohesion", "block"),
message=(
"this file mixes architectural concerns ("
+ ", ".join(sorted(kinds))
+ ") — each belongs in its own module"
),
fix_hint=(
"modularize: split this file so it owns a single concern — move "
"models, routes, and components into separate modules"
),
)
]
# --------------------------------------------------------------------------- #
# Python bodies — thin routes + god classes
# --------------------------------------------------------------------------- #
def _check_python_bodies(
rel_path: str, source: bytes, standard: ConventionsStandard
) -> list[Finding]:
root = get_parser("python").parse(source).root_node
findings: list[Finding] = []
for node in root.children:
func, decorators = _py_function(node)
if func is not None and _py_is_route(decorators) and _body_hits_db(func):
findings.append(_thin_route_finding(rel_path, func, standard))
cls = _py_class(node)
if cls is not None and _py_method_count(cls) > _GOD_CLASS_METHODS:
findings.append(_god_class_finding(rel_path, cls, standard))
return findings
def _thin_route_finding(
rel_path: str, func: Node, standard: ConventionsStandard
) -> Finding:
name = _text(func.child_by_field_name("name"))
return Finding(
file=rel_path,
line=func.start_point[0] + 1,
kind="route",
rule="thin_routes",
level=_level(standard, "thin_routes", "block"),
message=(
f"route '{name}' performs its own data access — a route should "
"delegate to a service / repository, not query the database"
),
fix_hint=(
"modularize: move the data-access logic into a service and call it "
"from the route"
),
)
def _god_class_finding(
rel_path: str, cls: Node, standard: ConventionsStandard
) -> Finding:
name = _text(cls.child_by_field_name("name"))
return Finding(
file=rel_path,
line=cls.start_point[0] + 1,
kind=None,
rule="god_class",
level=_level(standard, "god_class", "warn"),
message=(
f"class '{name}' has more than {_GOD_CLASS_METHODS} methods — it is "
"likely doing too much (single-responsibility)"
),
fix_hint="decompose: split the class along its distinct responsibilities",
)
def _py_function(node: Node) -> tuple[Node | None, list[Node]]:
if node.type == "decorated_definition":
inner = node.child_by_field_name("definition")
decorators = [c for c in node.children if c.type == "decorator"]
if inner is not None and inner.type == "function_definition":
return inner, decorators
return None, []
if node.type == "function_definition":
return node, []
return None, []
def _py_class(node: Node) -> Node | None:
if node.type == "decorated_definition":
inner = node.child_by_field_name("definition")
if inner is not None and inner.type == "class_definition":
return inner
return None
return node if node.type == "class_definition" else None
def _py_is_route(decorators: list[Node]) -> bool:
return any(_py_decorator_is_route(d) for d in decorators)
def _py_decorator_is_route(decorator: Node) -> bool:
expr = next((c for c in decorator.children if c.type != "@"), None)
if expr is not None and expr.type == "call":
expr = expr.child_by_field_name("function")
if expr is None or expr.type != "attribute":
return False
obj = expr.child_by_field_name("object")
method = _text(expr.child_by_field_name("attribute"))
obj_name = _text(obj) if obj is not None and obj.type == "identifier" else ""
return obj_name in _ROUTER_OBJECTS or method in _HTTP_METHODS
def _body_hits_db(func: Node) -> bool:
body = func.child_by_field_name("body")
if body is None:
return False
for call in _descendant_nodes(body, "call"):
fn = call.child_by_field_name("function")
if fn is None:
continue
if fn.type == "attribute":
if _text(fn.child_by_field_name("attribute")) in _DB_METHODS:
return True
elif fn.type == "identifier" and _text(fn) in _DB_CONSTRUCTS:
return True
return False
def _py_method_count(cls: Node) -> int:
body = cls.child_by_field_name("body")
if body is None:
return 0
count = 0
for child in body.children:
if child.type == "function_definition":
count += 1
elif child.type == "decorated_definition":
inner = child.child_by_field_name("definition")
if inner is not None and inner.type == "function_definition":
count += 1
return count
# --------------------------------------------------------------------------- #
# TypeScript bodies — thin components
# --------------------------------------------------------------------------- #
def _check_ts_bodies(
rel_path: str, source: bytes, language: str, standard: ConventionsStandard
) -> list[Finding]:
parser_language = "tsx" if language in ("typescript", "tsx") else language
root = get_parser(parser_language).parse(source).root_node
findings: list[Finding] = []
for func in _ts_component_functions(root):
if _component_fetches(func):
findings.append(_thin_component_finding(rel_path, func, standard))
return findings
def _thin_component_finding(
rel_path: str, func: Node, standard: ConventionsStandard
) -> Finding:
name = _text(func.child_by_field_name("name")) or "component"
return Finding(
file=rel_path,
line=func.start_point[0] + 1,
kind="component",
rule="thin_components",
level=_level(standard, "thin_components", "block"),
message=(
f"component '{name}' fetches data in its body — data fetching "
"belongs in a hook / query, not in a presentational component"
),
fix_hint=(
"modularize: extract the data fetching into a custom hook and "
"consume its result from the component"
),
)
def _ts_component_functions(root: Node) -> list[Node]:
components: list[Node] = []
for node in _descendant_nodes(root, "function_declaration"):
if _contains_jsx(node):
components.append(node)
for node in _descendant_nodes(root, "arrow_function"):
if _contains_jsx(node):
components.append(node)
return components
def _component_fetches(func: Node) -> bool:
body = func.child_by_field_name("body")
if body is None:
return False
for call in _descendant_nodes(body, "call_expression"):
fn = call.child_by_field_name("function")
if fn is None:
continue
if fn.type == "identifier" and _text(fn) in _FETCH_IDENTIFIERS:
return True
if fn.type == "member_expression":
obj = fn.child_by_field_name("object")
if obj is not None and _text(obj) in _FETCH_IDENTIFIERS:
return True
return False
def _contains_jsx(node: Node) -> bool:
body = node.child_by_field_name("body")
return any(True for _ in _descendant_nodes(body, *_JSX_TYPES)) if body else False
# --------------------------------------------------------------------------- #
# Shared AST helpers
# --------------------------------------------------------------------------- #
def _descendant_nodes(node: Node | None, *types: str) -> list[Node]:
if node is None:
return []
wanted = frozenset(types)
out: list[Node] = []
stack = list(node.children)
while stack:
current = stack.pop()
if current.type in wanted:
out.append(current)
stack.extend(current.children)
return out
def _text(node: Node | None) -> str:
if node is None or node.text is None:
return ""
return node.text.decode()
+2
View File
@@ -14,6 +14,7 @@ from . import classify_python, classify_ts
from .custom import check_custom
from .grammars import GrammarUnavailable
from .hygiene import check_hygiene
from .modularity import check_modularity
from .placement import check_placement
if TYPE_CHECKING:
@@ -61,6 +62,7 @@ def _check_file(
check_placement(rel, defs, standard)
+ check_hygiene(rel, source, language, standard)
+ check_custom(rel, source, language, standard)
+ check_modularity(rel, defs, source, language, standard)
)
+65 -5
View File
@@ -25,6 +25,7 @@ from roboco.foundation.policy.conventions.models import (
DefinitionKind,
Module,
Rule,
RuleLevel,
)
_IGNORE_DIRS = frozenset(
@@ -63,6 +64,16 @@ _MODULE_PATTERNS: tuple[tuple[frozenset[str], str, tuple[DefinitionKind, ...]],
"UI components",
("model", "route"),
),
(
frozenset({"hooks"}),
"React hooks",
("component", "model", "route"),
),
(
frozenset({"store", "stores", "state", "context", "contexts"}),
"state management",
("component", "route"),
),
(
frozenset({"helpers", "utils", "util", "lib"}),
"shared helpers / utilities",
@@ -79,16 +90,65 @@ _MAX_LIFTED_RULES = 25
def derive_from_scan(root: Path | str) -> ConventionsStandard:
"""Infer a conventions standard from the repository at ``root``."""
root_path = Path(root)
modules = _scan_modules(root_path)
languages = _detect_languages(root_path)
return ConventionsStandard(
languages=_detect_languages(root_path),
modules=_scan_modules(root_path),
rules=_seed_rules(),
languages=languages,
modules=modules,
rules=_seed_rules(modules, languages),
custom=_lift_claude_md(root_path),
)
def _seed_rules() -> dict[str, Rule]:
return {name: Rule(name=name, level=level) for name, level in BUILTIN_RULES.items()}
# Placement rules default to block — the level placement.py applies when a rule
# is absent — so auto-derived enforcement has teeth; the owner downgrades any
# rule to warn per-rule via the panel editor or the committed file.
_PLACEMENT_DEFAULT: RuleLevel = "block"
# Modularity rules — the separation-of-concerns checks that go beyond linting.
# Cohesion + god-class apply to any classifiable project; the body checks are
# stack-specific (thin routes for Python APIs, thin components for TS/React), so
# a Python project never carries thin_components and a frontend never carries
# thin_routes.
_MODULARITY_ANY: dict[str, RuleLevel] = {
"modular_cohesion": "block",
"god_class": "warn",
}
_MODULARITY_BY_LANGUAGE: dict[str, dict[str, RuleLevel]] = {
"python": {"thin_routes": "block"},
"typescript": {"thin_components": "block"},
}
def _seed_rules(modules: list[Module], languages: list[str]) -> dict[str, Rule]:
"""Seed the rules that actually apply to this project.
Three layers, each scoped so a project never carries a rule that cannot fire
on it:
- **Hygiene** (``BUILTIN_RULES``) — language-agnostic, seeded for everyone.
- **Placement** — seeded per detected module; the rule name mirrors the
validator's ``no_<kind>s_in_<leaf>``, so a frontend repo gets
``no_models_in_components`` rather than a backend ``no_models_in_routers``.
- **Modularity** — the separation-of-concerns checks; cohesion + god-class
for any stack, plus the stack-specific body checks (thin routes for
Python, thin components for TypeScript).
"""
rules = {
name: Rule(name=name, level=level) for name, level in BUILTIN_RULES.items()
}
for module in modules:
leaf = module.path.rstrip("/").rsplit("/", 1)[-1]
for kind in module.forbidden:
name = f"no_{kind}s_in_{leaf}"
rules.setdefault(name, Rule(name=name, level=_PLACEMENT_DEFAULT))
if languages:
for name, any_level in _MODULARITY_ANY.items():
rules.setdefault(name, Rule(name=name, level=any_level))
for language in languages:
for name, lang_level in _MODULARITY_BY_LANGUAGE.get(language, {}).items():
rules.setdefault(name, Rule(name=name, level=lang_level))
return rules
def _walk_dirs(root: Path) -> list[tuple[str, list[str], list[str]]]:
@@ -29,12 +29,13 @@ class ConventionsParseError(ValueError):
self.reason = reason
# The org-default rule set: applied to every project's effective map before the
# committed file or auto-derived rules overlay it. Keep in sync with the
# validator's rule emitters and the panel's rule list.
# The org-default HYGIENE rules — language-agnostic, applicable to any project
# regardless of stack, so they seed into every project's effective map. Placement
# rules (e.g. no_models_in_routers) are NOT here: they only make sense where the
# target module exists, so they are derived per-project from the repo scan (see
# roboco/conventions/scan.py). This is why a frontend project never shows a
# backend "no models in routers" rule.
BUILTIN_RULES: dict[str, RuleLevel] = {
"no_models_in_routers": "block",
"no_helpers_in_routers": "block",
"no_lint_suppressions": "block",
"no_inline_comments": "warn",
}
+110
View File
@@ -0,0 +1,110 @@
"""Modularity checks: cohesion, thin routes, thin components, god class."""
from __future__ import annotations
from roboco.conventions import classify_python, classify_ts
from roboco.conventions.modularity import check_modularity
from roboco.foundation.policy.conventions.models import ConventionsStandard
def _py(src: str) -> set[str]:
source = src.encode()
defs = classify_python.classify_definitions(source)
findings = check_modularity(
"app/x.py", defs, source, "python", ConventionsStandard()
)
return {f.rule for f in findings}
def _ts(src: str, lang: str = "tsx") -> set[str]:
source = src.encode()
defs = classify_ts.classify_definitions(source, lang)
findings = check_modularity("src/x.tsx", defs, source, lang, ConventionsStandard())
return {f.rule for f in findings}
# --- Cohesion: one architectural concern per file --------------------------- #
def test_cohesion_flags_model_and_route_in_one_file() -> None:
rules = _py(
"from pydantic import BaseModel\n"
"from fastapi import APIRouter\n"
"router = APIRouter()\n"
"class UserIn(BaseModel):\n"
" name: str\n"
"@router.post('/users')\n"
"def create_user(u):\n"
" return u\n"
)
assert "modular_cohesion" in rules
def test_cohesion_clean_for_a_single_concern() -> None:
rules = _py(
"from fastapi import APIRouter\n"
"router = APIRouter()\n"
"@router.get('/a')\n"
"def a():\n return 1\n"
"@router.get('/b')\n"
"def b():\n return 2\n"
)
assert "modular_cohesion" not in rules
# --- Thin routes: a route must delegate, not query the DB ------------------- #
def test_thin_routes_flags_db_access_in_route() -> None:
rules = _py(
"from fastapi import APIRouter\n"
"router = APIRouter()\n"
"@router.get('/users')\n"
"def list_users(db):\n"
" return db.execute('select 1').scalars().all()\n"
)
assert "thin_routes" in rules
def test_thin_routes_clean_when_delegating_to_a_service() -> None:
rules = _py(
"from fastapi import APIRouter\n"
"router = APIRouter()\n"
"@router.get('/users')\n"
"def list_users(svc):\n"
" return svc.list_users()\n"
)
assert "thin_routes" not in rules
# --- Thin components: data fetching belongs in a hook ----------------------- #
def test_thin_components_flags_fetch_in_component() -> None:
rules = _ts(
"export function UserList() {\n"
" const data = fetch('/api/users');\n"
" return <div>{data}</div>;\n"
"}\n"
)
assert "thin_components" in rules
def test_thin_components_clean_when_presentational() -> None:
rules = _ts(
"export function UserList(props) {\n return <ul>{props.users}</ul>;\n}\n"
)
assert "thin_components" not in rules
# --- God class: single responsibility --------------------------------------- #
def test_god_class_flags_a_class_with_too_many_methods() -> None:
methods = "\n".join(f" def m{i}(self):\n return {i}" for i in range(16))
assert "god_class" in _py("class Big:\n" + methods + "\n")
def test_god_class_clean_for_a_small_class() -> None:
rules = _py("class Small:\n def a(self):\n return 1\n")
assert "god_class" not in rules
@@ -81,11 +81,13 @@ def test_unknown_definition_kind_in_forbidden_raises() -> None:
)
def test_builtin_rules_cover_the_org_defaults() -> None:
assert BUILTIN_RULES["no_models_in_routers"] == "block"
assert BUILTIN_RULES["no_helpers_in_routers"] == "block"
def test_builtin_rules_are_language_agnostic_hygiene_only() -> None:
# BUILTIN_RULES are the universal hygiene defaults; placement / modularity
# rules are derived per project from the scan, never seeded universally.
assert BUILTIN_RULES["no_lint_suppressions"] == "block"
assert BUILTIN_RULES["no_inline_comments"] == "warn"
assert "no_models_in_routers" not in BUILTIN_RULES
assert "no_helpers_in_routers" not in BUILTIN_RULES
def test_models_construct_directly() -> None:
@@ -14,8 +14,10 @@ from roboco.foundation.policy.conventions.models import (
def test_effective_map_applies_builtin_rules_when_file_absent() -> None:
eff = effective_map(ConventionsStandard(), None)
assert eff.rules["no_models_in_routers"].level == "block"
assert eff.rules["no_lint_suppressions"].level == "block"
assert eff.rules["no_inline_comments"].level == "warn"
# Placement / modularity rules are derived per project, not universal.
assert "no_models_in_routers" not in eff.rules
def test_file_module_overrides_derived_by_path() -> None: