From 4a8c508f912c505d38fd4c079c125c5c0cfe8150 Mon Sep 17 00:00:00 2001 From: Renn F Date: Mon, 22 Jun 2026 14:26:25 +0200 Subject: [PATCH] feat(conventions): modularity checks + scan-derived, language-aware rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- roboco/conventions/modularity.py | 342 ++++++++++++++++++ roboco/conventions/runner.py | 2 + roboco/conventions/scan.py | 70 +++- .../foundation/policy/conventions/models.py | 11 +- tests/unit/conventions/test_modularity.py | 110 ++++++ .../conventions/test_conventions_models.py | 8 +- .../policy/conventions/test_effective_map.py | 4 +- 7 files changed, 533 insertions(+), 14 deletions(-) create mode 100644 roboco/conventions/modularity.py create mode 100644 tests/unit/conventions/test_modularity.py diff --git a/roboco/conventions/modularity.py b/roboco/conventions/modularity.py new file mode 100644 index 00000000..5c691ae7 --- /dev/null +++ b/roboco/conventions/modularity.py @@ -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() diff --git a/roboco/conventions/runner.py b/roboco/conventions/runner.py index 4e7bb656..b93e9720 100644 --- a/roboco/conventions/runner.py +++ b/roboco/conventions/runner.py @@ -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) ) diff --git a/roboco/conventions/scan.py b/roboco/conventions/scan.py index 3a02aef5..3421b50d 100644 --- a/roboco/conventions/scan.py +++ b/roboco/conventions/scan.py @@ -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_s_in_``, 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]]]: diff --git a/roboco/foundation/policy/conventions/models.py b/roboco/foundation/policy/conventions/models.py index 5d295bfa..b6458bdf 100644 --- a/roboco/foundation/policy/conventions/models.py +++ b/roboco/foundation/policy/conventions/models.py @@ -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", } diff --git a/tests/unit/conventions/test_modularity.py b/tests/unit/conventions/test_modularity.py new file mode 100644 index 00000000..6791ee8e --- /dev/null +++ b/tests/unit/conventions/test_modularity.py @@ -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
{data}
;\n" + "}\n" + ) + assert "thin_components" in rules + + +def test_thin_components_clean_when_presentational() -> None: + rules = _ts( + "export function UserList(props) {\n return
    {props.users}
;\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 diff --git a/tests/unit/foundation/policy/conventions/test_conventions_models.py b/tests/unit/foundation/policy/conventions/test_conventions_models.py index 4bfef2b0..c710be97 100644 --- a/tests/unit/foundation/policy/conventions/test_conventions_models.py +++ b/tests/unit/foundation/policy/conventions/test_conventions_models.py @@ -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: diff --git a/tests/unit/foundation/policy/conventions/test_effective_map.py b/tests/unit/foundation/policy/conventions/test_effective_map.py index b27369be..f145ad45 100644 --- a/tests/unit/foundation/policy/conventions/test_effective_map.py +++ b/tests/unit/foundation/policy/conventions/test_effective_map.py @@ -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: