From 8e00969c3e40b3ca9d82f272875b252f3326ce5f Mon Sep 17 00:00:00 2001 From: Sergey <93754860+slaid098@users.noreply.github.com> Date: Tue, 4 Aug 2026 13:31:15 +0300 Subject: [PATCH] feat(skills): code-standards architecture + tests sections, scattered-models check (#256) * feat(skills): code-standards architecture + tests sections * feat(infra): scattered-models AST check in project-status * test(infra): scattered-models check tests (10 cases) --------- Co-authored-by: opencode-agent --- .opencode/scripts/project-status.py | 124 +++++++++++++++++++ .opencode/skills/code-standards/SKILL.md | 110 +++++++++++++++++ tests/test_project_status.py | 151 +++++++++++++++++++++++ 3 files changed, 385 insertions(+) diff --git a/.opencode/scripts/project-status.py b/.opencode/scripts/project-status.py index 8969e2e..56b5faa 100644 --- a/.opencode/scripts/project-status.py +++ b/.opencode/scripts/project-status.py @@ -410,6 +410,129 @@ def _check_flat_layout(ptype: ProjectType, ctx: RepoCtx) -> CheckResult | None: ) +def _is_tortoise_model_base(base: ast.expr) -> bool: + """True if a class base references a Tortoise/ORM ``Model``. + + Matches ``Model``, ``tortoise.Model``, ``models.Model``, ``tortoise.models.Model``. + Does NOT match unrelated ``Model`` classes from other libraries — the caller + is expected to have already verified the file imports ``tortoise`` or its + own ``db.models`` module. Used by ``_check_scattered_models`` to detect + ORM models living outside ``src//db/models/``. + """ + # bare ``Model`` (Name) — common when ``from tortoise import Model`` + if isinstance(base, ast.Name) and base.id == "Model": + return True + # ``tortoise.Model`` / ``models.Model`` / ``tortoise.models.Model`` (Attribute) + if isinstance(base, ast.Attribute): + # last attribute segment must be ``Model``; accept any qualifier + # (``tortoise``, ``models``, ``db.models``, ...) — the import is + # validated separately by the caller's import check. + return base.attr == "Model" + return False + + +def _file_imports_tortoise_or_models(source: str, pkg: str) -> bool: + """True if the file imports ``tortoise`` or its own ``db.models`` module. + + Used to suppress false positives in ``_check_scattered_models``: a class + named ``Model`` from an unrelated library (e.g. ``pydantic.BaseModel`` is + already excluded by name, but other libs may define their own ``Model``) + should not trigger the WARN unless the file actually uses Tortoise/ORM. + + The check is AST-based (walks ``ast.Import`` and ``ast.ImportFrom``) so it + is robust to comments and strings mentioning those names. + """ + try: + tree = ast.parse(source) + except SyntaxError: + return False + models_module = f"{pkg}.db.models" + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "tortoise" or alias.name.startswith("tortoise."): + return True + elif isinstance(node, ast.ImportFrom): + module = node.module or "" + if module == "tortoise" or module.startswith("tortoise."): + return True + if module == models_module or module.startswith(models_module + "."): + return True + return False + + +def _scan_file_for_models(py_file: Path, ctx: RepoCtx, pkg: str) -> list[CheckResult]: + """Scan a single ``.py`` file for ORM ``class X(Model)`` definitions. + + Returns one ``WARN`` CheckResult per offending class (with the file's + relative path). Returns ``[]`` if the file does not import tortoise or + the package's own ``db.models`` module (false-positive guard), or if no + class inherits from ``Model``. + """ + try: + source = py_file.read_text(encoding="utf-8-sig") + except OSError: + return [] + if not _file_imports_tortoise_or_models(source, pkg): + return [] + try: + tree = ast.parse(source) + except SyntaxError: + return [] + results: list[CheckResult] = [] + rel = py_file.relative_to(ctx.root) + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + for base in node.bases: + if _is_tortoise_model_base(base): + results.append( + CheckResult( + CheckStatus.WARN, + str(rel), + f"class {node.name}(Model) вне db/models/ — " + "models must live in src//db/models/", + ) + ) + break + return results + + +def _check_scattered_models(ctx: RepoCtx, ptype: ProjectType) -> list[CheckResult]: + """AST-чек: ORM models должны жить в ``src//db/models/``. + + Scans ``.py`` files in ``src//`` (excluding ``db/models/``) for + ``class X(Model)`` / ``class X(tortoise.Model)``. If found → WARN with the + offending relative path. Models in ``db/models/`` are OK; files that do not + import ``tortoise`` or the package's own ``db.models`` module are skipped + (suppresses false positives from unrelated libraries defining ``Model``). + + Skips for non-backend/fullstack types, flat layouts (no ``src//``), + and repos without a resolvable package name. Returns ``[]`` in all skip + cases so the caller appends nothing. + """ + if ptype not in {ProjectType.BACKEND, ProjectType.FULLSTACK}: + return [] + pkg = _resolve_package_name(ctx) + if pkg is None: + return [] + # For FULLSTACK, the backend lives under ``backend/``; for BACKEND, at root. + src_pkg = ctx.root / "src" / pkg + if ptype == ProjectType.FULLSTACK: + backend_src = ctx.root / "backend" / "src" / pkg + if backend_src.is_dir(): + src_pkg = backend_src + if not src_pkg.is_dir(): + return [] + models_dir = src_pkg / "db" / "models" + results: list[CheckResult] = [] + for py_file in src_pkg.rglob("*.py"): + if models_dir in py_file.parents or py_file == models_dir: + continue + results.extend(_scan_file_for_models(py_file, ctx, pkg)) + return results + + def _check_type_specific_structure(ptype: ProjectType, ctx: RepoCtx) -> list[CheckResult]: """Type-specific extra checks beyond the expected dirs list.""" results: list[CheckResult] = [] @@ -424,6 +547,7 @@ def _check_type_specific_structure(ptype: ProjectType, ctx: RepoCtx) -> list[Che flat = _check_flat_layout(ptype, ctx) if flat is not None: results.append(flat) + results.extend(_check_scattered_models(ctx, ptype)) return results diff --git a/.opencode/skills/code-standards/SKILL.md b/.opencode/skills/code-standards/SKILL.md index ac38372..e395fba 100644 --- a/.opencode/skills/code-standards/SKILL.md +++ b/.opencode/skills/code-standards/SKILL.md @@ -25,3 +25,113 @@ description: Universal code standards for any language. Use when writing, refact - AGENTS.md правило «No comments unless requested» — это **default**: код без комментариев - Этот skill описывает **исключение**: Google-style docstrings на английском для публичных API — когда контракт warrants (библиотечный API, public surface) - Описывай **зачем**, а не **что** — код и так говорит что делает + +## 5. Architecture: good vs bad + +Слои для **backend**: `routes → schemas → services → db/models` (4-tier, однонаправленный). Роуты тонкие (импортируют только `services` + `schemas`), сервисы работают с `db/models`, бизнес-логика здесь. `project-status.py` enforces subset (thin routes, centralized models); этот раздел объясняет «почему». + +### Backend (подробно) + +**GOOD tree (синтетический):** +``` +src// +├── api/ +│ ├── v1/ +│ │ ├── routes/users.py ← тонкие роуты, импортируют только services + schemas +│ │ ├── dependencies.py ← Depends(), get_current_user +│ │ └── router.py +│ └── router.py +├── config/ +│ ├── settings.py ← pydantic-settings +│ └── logger.py ← loguru setup +├── db/ +│ ├── connection.py ← Tortoise.init +│ └── models/ ← ВСЕ ORM-модели здесь (centralized) +│ ├── user.py +│ ├── post.py +│ └── comment.py +├── schemas/ ← Pydantic DTO (НЕ Tortoise models) +│ ├── base.py +│ ├── user.py +│ └── post.py +├── services/ ← бизнес-логика (работает с db/models) +│ ├── user_service.py +│ └── post_service.py +└── utils/ + └── metadata.py +``` + +**Правила GOOD:** +1. Все ORM-модели в `db/models/` (centralized) +2. Schemas (Pydantic) отдельно от models (Tortoise) — НЕ смешивать +3. Роуты тонкие — импортируют только `services` и `schemas` +4. Сервисы работают с `db/models` — бизнес-логика здесь +5. Слои: routes → schemas → services → db/models (4-tier, однонаправленный) + +**BAD tree 1 — feature-scatter:** +``` +src// +├── channels/ +│ ├── models.py ← ❌ модель здесь (scatter) +│ ├── routes.py +│ └── service.py +├── monitor/ +│ ├── models.py ← ❌ ещё модель здесь +│ └── routes.py +├── logs/ +│ └── models.py ← ❌ и здесь +├── models.py ← ❌ root-level модель +├── db.py ← ❌ connection flat (не db/connection.py) +└── main.py +``` + +Проблемы BAD 1: модели раскиданы по feature-папкам; `models.py` в root; `db.py` flat; не publishable; Tortoise `modules` должен перечислять 4+ файла вручную. + +**BAD tree 2 — mixed-layers:** +``` +src// +├── api/ +│ ├── v1/ +│ │ └── users.py ← ❌ роут содержит бизнес-логику + Tortoise queries +│ └── models.py ← ❌ модели в api/ (не в db/models/) +├── services/ +│ └── user_service.py +│ └── schemas.py ← ❌ schemas в services/ (не в schemas/) +└── main.py +``` + +Проблемы BAD 2: роут делает Tortoise queries напрямую (не тонкий); модели в `api/models.py` (не `db/models/`); schemas внутри services (не отдельный слой). + +### Fullstack (кратко) + +Backend as above (in `backend/` + `frontend/` separation). Frontend: SvelteKit co-located `*.test.ts` в `src/lib/`, `e2e/*.spec.ts` для Playwright. НЕ смешивать backend код в `frontend/` и наоборот. + +### CLI (кратко) + +`cli.py` (Typer commands) + `core.py` (business logic). Нет api/db/schemas layers. `tests/test_cli.py` + `tests/test_core.py`. + +## 6. Tests + +Что писать (как запускать — в `run-tests` skill). `project-status.py` enforces subset (conftest required, anti-stub, mirror structure); этот раздел объясняет «почему». + +### Типы тестов + +- **Regression** — воспроизводит конкретный баг, который был исправлен. Ссылается на issue/PR (`test_parser_handles_crlf_regression_#227`). +- **Integration** — пересекает слои (DB+API, scheduler+DB). Имеет `pytest.mark.integration` + `skipif` opt-in. В `tests/integration/`. +- **Unit** — чистая функция/сервис, без DB/сети. В `tests/unit/`. + +### Антипаттерны + +- **Stub files** — `test_*.py` без `def test_*`/`async def test_*` (digital_factory 50/62 файлов). `project-status` WARNs. +- **Тесты без assertions** — только `print`/`logger.info`. Каждый тест должен иметь минимум 1 `assert`. +- **Тесты ради тестов** — coverage ради coverage, без реальной проверки поведения. + +### Mirror structure (backend) + +- `tests/unit/` ↔ `src//services/` (unit-тесты сервисов) +- `tests/api/` ↔ `src//api/v1/routes/` (route-тесты через TestClient) +- `tests/integration/` ↔ cross-cutting flows (opt-in) + +### conftest.py + +Обязателен (backend). Shared fixtures: `mock_settings`, `client`, `auth_client`, `create_` factories. diff --git a/tests/test_project_status.py b/tests/test_project_status.py index a85f595..cd74f48 100644 --- a/tests/test_project_status.py +++ b/tests/test_project_status.py @@ -1852,3 +1852,154 @@ def test_load_pyproject_parse_error_sentinel(tmp_path, ctx): group = ps.check_pyproject(ps.ProjectType.BACKEND, ctx) assert group.overall() == ps.CheckStatus.FAIL assert any("парсинг" in c.detail for c in group.checks) + + +# ── scattered-models AST check (issue #251) ───────────────────────────────── + + +def _make_centralized_models_repo(tmp_path: Path) -> None: + """Backend repo with all ORM models centralized in ``src//db/models/``. + + Helper for scattered-models tests: produces the GOOD layout (no WARN). + """ + _make_backend_repo(tmp_path) + pkg = "test_repo" + (tmp_path / f"src/{pkg}/db/models/__init__.py").write_text("") + (tmp_path / f"src/{pkg}/db/models/user.py").write_text( + "from tortoise import Model, fields\nclass User(Model):\n name = fields.CharField()\n" + ) + + +def test_scattered_models_centralized_ok(tmp_path, ctx): + """All ORM models in ``src//db/models/`` → no scattered-models WARN.""" + _make_centralized_models_repo(tmp_path) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert results == [], f"expected no WARN, got: {results}" + + +def test_scattered_models_feature_scatter_warn(tmp_path, ctx): + """``class X(Model)`` in ``src//channels/models.py`` → WARN per file.""" + _make_centralized_models_repo(tmp_path) + pkg = "test_repo" + scattered_files = [ + f"src/{pkg}/channels/models.py", + f"src/{pkg}/monitor/models.py", + f"src/{pkg}/logs/models.py", + f"src/{pkg}/models.py", + ] + for rel in scattered_files: + path = tmp_path / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + "from tortoise import Model, fields\n" + "class Thing(Model):\n name = fields.CharField()\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert len(results) == 4, f"expected 4 WARN, got {len(results)}: {results}" + warned_paths = {r.name for r in results} + for rel in scattered_files: + assert rel in warned_paths, f"missing WARN for {rel}" + assert all(r.status == ps.CheckStatus.WARN for r in results) + + +def test_scattered_models_in_db_connection_warn(tmp_path, ctx): + """Model in ``src//db/connection.py`` (inside db/, not models/) → WARN.""" + _make_centralized_models_repo(tmp_path) + pkg = "test_repo" + (tmp_path / f"src/{pkg}/db/connection.py").write_text( + "from tortoise import Model, fields\nclass Internal(Model):\n x = fields.IntField()\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert len(results) == 1, f"expected 1 WARN, got: {results}" + assert results[0].name == f"src/{pkg}/db/connection.py" + assert results[0].status == ps.CheckStatus.WARN + + +def test_scattered_models_in_tests_ok(tmp_path, ctx): + """Model in ``tests/`` (test model) → OK (skip, tests/ not in src//).""" + _make_centralized_models_repo(tmp_path) + (tmp_path / "tests/test_models.py").write_text( + "from tortoise import Model, fields\n" + "class TestModel(Model):\n x = fields.IntField()\n" + "def test_model(): assert TestModel\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert results == [], f"expected no WARN, got: {results}" + + +def test_scattered_models_flat_layout_skip(tmp_path, ctx): + """Flat layout (no ``src//``) → skip check, no results.""" + _write_pyproject(tmp_path, deps=["fastapi", "uvicorn"]) + (tmp_path / "src").mkdir() + (tmp_path / "src/models.py").write_text( + "from tortoise import Model, fields\nclass Flat(Model):\n x = fields.IntField()\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert results == [], f"flat layout should skip, got: {results}" + + +def test_scattered_models_cli_skip(tmp_path, ctx): + """CLI (no db) → skip scattered-models check (returns []).""" + (tmp_path / "src").mkdir() + pkg = tmp_path / "src" / "mycli" + pkg.mkdir() + (pkg / "__init__.py").write_text("") + (pkg / "cli.py").write_text("from tortoise import Model\nclass CliModel(Model):\n pass\n") + _write_pyproject(tmp_path, deps=["typer"], scripts={"mycli": "mycli.cli:main"}, name="mycli") + results = ps._check_scattered_models(ctx, ps.ProjectType.CLI) + assert results == [], f"CLI should skip, got: {results}" + + +def test_scattered_models_no_tortoise_import_skip(tmp_path, ctx): + """File with ``class X(Model)`` but no tortoise import → skip (false positive guard).""" + _make_centralized_models_repo(tmp_path) + pkg = "test_repo" + (tmp_path / f"src/{pkg}/services/other.py").write_text( + "# class from another lib defining its own Model\n" + "from some_other_lib import Model\n" + "class Other(Model):\n pass\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert results == [], f"non-tortoise Model should not WARN, got: {results}" + + +def test_scattered_models_via_import_from_models_module(tmp_path, ctx): + """``class X(Model)`` where ``Model`` comes from own ``db.models`` → WARN.""" + _make_centralized_models_repo(tmp_path) + pkg = "test_repo" + (tmp_path / f"src/{pkg}/services/user_service.py").write_text( + f"from {pkg}.db.models import Model\nclass ServiceModel(Model):\n pass\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert len(results) == 1, f"expected 1 WARN, got: {results}" + assert results[0].name == f"src/{pkg}/services/user_service.py" + assert results[0].status == ps.CheckStatus.WARN + + +def test_scattered_models_check_structure_integration(tmp_path, ctx): + """``check_structure`` for BACKEND includes scattered-models WARNs.""" + _make_centralized_models_repo(tmp_path) + pkg = "test_repo" + (tmp_path / f"src/{pkg}/channels").mkdir(parents=True, exist_ok=True) + (tmp_path / f"src/{pkg}/channels/models.py").write_text( + "from tortoise import Model, fields\nclass Chan(Model):\n name = fields.CharField()\n" + ) + group = ps.check_structure(ps.ProjectType.BACKEND, ctx) + scattered_warns = [ + c + for c in group.checks + if "channels/models.py" in c.name and c.status == ps.CheckStatus.WARN + ] + assert scattered_warns, "expected scattered WARN in check_structure, got: " + ", ".join( + f"{c.name}={c.status.value}" for c in group.checks + ) + + +def test_scattered_models_no_pyproject_skip(tmp_path, ctx): + """No pyproject.toml (cannot resolve package name) → skip, no results.""" + (tmp_path / "src").mkdir() + (tmp_path / "src/whatever.py").write_text( + "from tortoise import Model\nclass X(Model):\n pass\n" + ) + results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND) + assert results == [], f"no pyproject should skip, got: {results}"