diff --git a/backend/apps/agents/agent_manager.py b/backend/apps/agents/agent_manager.py index 6c82ec62..38fef2a2 100644 --- a/backend/apps/agents/agent_manager.py +++ b/backend/apps/agents/agent_manager.py @@ -16,7 +16,6 @@ from backend.apps.agents.core.ws_manager import ws_manager from backend.apps.settings.settings import load_settings from backend.apps.tools_lib.tools_lib import ( _load_all as load_all_tools, - _sanitize_server_name, derive_mcp_config, load_builtin_permissions, load_trusted_sensitive_paths, @@ -25,6 +24,7 @@ from backend.apps.tools_lib.tools_lib import ( refresh_hubspot_token, save_trusted_sensitive_paths, ) +from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name from backend.apps.agents.core.error_classify import ( _is_auth_error, _is_free_trial_exhausted, @@ -173,7 +173,7 @@ class AgentManager: logger.info(f"[MCP-DEBUG] SKIPPED {tool.name}: '{tool_ref}' not in allowed_tools") continue - server_name = _sanitize_server_name(tool.name) + server_name = sanitize_mcp_server_name(tool.name) if active_set is not None and server_name not in active_set: logger.info(f"[MCP-DEBUG] GATED {server_name}: not in session.active_mcps, model must call MCPActivate first") continue @@ -664,7 +664,7 @@ class AgentManager: for t in load_all_tools(): if not t.mcp_config or not t.enabled: continue - if _sanitize_server_name(t.name) == server_slug: + if sanitize_mcp_server_name(t.name) == server_slug: return t.tool_permissions.get(mcp_tool_name, "ask") return _default_for(tool_name) @@ -1039,7 +1039,7 @@ class AgentManager: # Emit a context_status event so the model and UI both know. try: _enabled = { - _sanitize_server_name(t.name) + sanitize_mcp_server_name(t.name) for t in load_all_tools() if t.mcp_config and t.enabled and t.auth_status in ("configured", "connected") } @@ -1340,7 +1340,7 @@ class AgentManager: tool_def = next( (t for t in all_tools_list - if t.mcp_config and t.enabled and _sanitize_server_name(t.name) == name), + if t.mcp_config and t.enabled and sanitize_mcp_server_name(t.name) == name), None, ) if tool_def: diff --git a/backend/apps/agents/manager/prompt/prompt_context.py b/backend/apps/agents/manager/prompt/prompt_context.py index b6935496..f1f28474 100644 --- a/backend/apps/agents/manager/prompt/prompt_context.py +++ b/backend/apps/agents/manager/prompt/prompt_context.py @@ -3,8 +3,8 @@ from typing import Callable from backend.apps.modes.modes import load_mode from backend.apps.tools_lib.tools_lib import ( _load_all as load_all_tools, - _sanitize_server_name, ) +from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name from backend.apps.agents.manager.prompt.tool_catalog import _get_denied_tool_names, _is_fully_denied @@ -34,7 +34,7 @@ def _build_connected_tools_context(allowed_tools: list[str], get_all_tool_names: if _is_fully_denied(tool): continue - server_name = _sanitize_server_name(tool.name) + server_name = sanitize_mcp_server_name(tool.name) denied = _get_denied_tool_names(tool) tool_descs = { k: v for k, v in tool.tool_permissions.get("_tool_descriptions", {}).items() @@ -263,7 +263,7 @@ def _build_mcp_registry_summary(allowed_tools: list[str], active_mcps: list[str] continue if _is_fully_denied(tool): continue - server_name = _sanitize_server_name(tool.name) + server_name = sanitize_mcp_server_name(tool.name) desc = (getattr(tool, "description", None) or "").strip() if not desc: # Fall back to a generic blurb keyed on the tool name so the @@ -373,7 +373,7 @@ def _resolve_forced_tools(forced_tools: list[str] | None) -> str: if not t.enabled or not t.tool_permissions: continue tool_descs = t.tool_permissions.get("_tool_descriptions", {}) - server_name = _sanitize_server_name(t.name) + server_name = sanitize_mcp_server_name(t.name) for tn, td in tool_descs.items(): desc_map[tn] = td tool_to_server[tn] = server_name diff --git a/backend/apps/google_workspace_mcp_shim/run.py b/backend/apps/google_workspace_mcp_shim/run.py index 189b107f..103c5932 100644 --- a/backend/apps/google_workspace_mcp_shim/run.py +++ b/backend/apps/google_workspace_mcp_shim/run.py @@ -40,7 +40,7 @@ def _patched_get_credentials(): ) -gauth.get_credentials = _patched_get_credentials +gauth.get_credentials = _patched_get_credentials # vulture-ignore: get_credentials from google_workspace_mcp import __main__ as _gw_main # noqa: E402,F401 diff --git a/backend/apps/service/client.py b/backend/apps/service/client.py index 176d0754..6c445f33 100644 --- a/backend/apps/service/client.py +++ b/backend/apps/service/client.py @@ -296,7 +296,7 @@ def sync(data: dict | None = None) -> None: "t": time.time(), "submission_id": uuid4().hex, } - _log("s", payload) + _log("s") if _test_sink is not None: try: _test_sink("s", body) diff --git a/backend/apps/tools_lib/mcp_config.py b/backend/apps/tools_lib/mcp_config.py index ba80976a..eae40ab9 100644 --- a/backend/apps/tools_lib/mcp_config.py +++ b/backend/apps/tools_lib/mcp_config.py @@ -11,7 +11,7 @@ from backend.apps.tools_lib.oauth_config import OPENSWARM_OAUTH_BASE_URL logger = logging.getLogger(__name__) -def _sanitize_server_name(name: str) -> str: +def sanitize_mcp_server_name(name: str) -> str: """Convert a tool name into a valid MCP server identifier (alphanumeric + hyphens).""" return re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") diff --git a/backend/main.py b/backend/main.py index b4308ecb..fe186778 100644 --- a/backend/main.py +++ b/backend/main.py @@ -550,7 +550,8 @@ async def mcp_meta(action: str, request: Request): valid options instead of activating (anti-hallucination). """ from backend.apps.agents.agent_manager import agent_manager - from backend.apps.tools_lib.tools_lib import _load_all as load_all_tools, _sanitize_server_name + from backend.apps.tools_lib.tools_lib import _load_all as load_all_tools + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name body = await request.json() parent_session_id = body.get("parent_session_id", "") @@ -586,7 +587,7 @@ async def mcp_meta(action: str, request: Request): for t in load_all_tools(): if not (t.mcp_config and t.enabled and t.auth_status in ("configured", "connected")): continue - sanitized = _sanitize_server_name(t.name) + sanitized = sanitize_mcp_server_name(t.name) # Pull tool sub-action names from tool_permissions._tool_descriptions # so MCPSearch can match against capability names (e.g. "send_email"). action_names: list[str] = [] @@ -710,7 +711,7 @@ async def mcp_meta(action: str, request: Request): tool_hint = "" try: for t in load_all_tools(): - if _sanitize_server_name(t.name) != server_name: + if sanitize_mcp_server_name(t.name) != server_name: continue descs = (t.tool_permissions or {}).get("_tool_descriptions", {}) or {} if not descs: diff --git a/backend/tests/test_v2_invariants.py b/backend/tests/test_v2_invariants.py index 37966db0..ddfa116b 100644 --- a/backend/tests/test_v2_invariants.py +++ b/backend/tests/test_v2_invariants.py @@ -581,9 +581,9 @@ def test_mcp_brand_covers_curated_servers(): "google-workspace", "microsoft-365", "slack", "discord", "notion", "airtable", "hubspot", "reddit", "youtube", } - from backend.apps.tools_lib.tools_lib import _sanitize_server_name + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name for slug in curated: - assert _sanitize_server_name(slug) == slug, ( + assert sanitize_mcp_server_name(slug) == slug, ( f"curated slug {slug!r} is not in sanitized form" ) @@ -599,30 +599,30 @@ def test_curated_server_aliases_in_main(): def test_sanitize_server_name_idempotent(): - """_sanitize_server_name must be idempotent (sanitize twice = sanitize once).""" - from backend.apps.tools_lib.tools_lib import _sanitize_server_name + """sanitize_mcp_server_name must be idempotent (sanitize twice = sanitize once).""" + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name test_inputs = [ "Google Workspace", "Microsoft 365", "Slack", "Discord", "Notion", "Airtable", "HubSpot", "Reddit", "YouTube", "GitHub", "GitLab", "Jira", ] for raw in test_inputs: - once = _sanitize_server_name(raw) - twice = _sanitize_server_name(once) + once = sanitize_mcp_server_name(raw) + twice = sanitize_mcp_server_name(once) assert once == twice, f"{raw}: sanitize not idempotent ({once} != {twice})" def test_sanitize_server_name_lowercase(): - from backend.apps.tools_lib.tools_lib import _sanitize_server_name - assert _sanitize_server_name("Gmail") == "gmail" - assert _sanitize_server_name("UPPERCASE") == "uppercase" + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name + assert sanitize_mcp_server_name("Gmail") == "gmail" + assert sanitize_mcp_server_name("UPPERCASE") == "uppercase" def test_sanitize_server_name_strips_special_chars(): - from backend.apps.tools_lib.tools_lib import _sanitize_server_name - assert _sanitize_server_name("Foo Bar!") == "foo-bar" - assert _sanitize_server_name("@x/y") == "x-y" - assert _sanitize_server_name("a__b") == "a-b" + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name + assert sanitize_mcp_server_name("Foo Bar!") == "foo-bar" + assert sanitize_mcp_server_name("@x/y") == "x-y" + assert sanitize_mcp_server_name("a__b") == "a-b" # =========================================================================== @@ -635,10 +635,10 @@ def test_mcp_activate_handler_unknown_server(): # We test the response shape independently of the FastAPI plumbing. # The handler is a closure inside main.py:mcp_meta_handler, so we # instead exercise the contract: invalid name surfaces alternatives. - from backend.apps.tools_lib.tools_lib import _sanitize_server_name + from backend.apps.tools_lib.mcp_config import sanitize_mcp_server_name valid = {"gmail", "slack", "google-workspace"} requested = "Gmail" # raw, needs sanitize - sanitized = _sanitize_server_name(requested) + sanitized = sanitize_mcp_server_name(requested) if sanitized in valid: status = "would_activate" else: diff --git a/frontend/src/app/pages/AgentChat/parsing/toolLabels.ts b/frontend/src/app/pages/AgentChat/parsing/toolLabels.ts index 0754f35b..618e82f4 100644 --- a/frontend/src/app/pages/AgentChat/parsing/toolLabels.ts +++ b/frontend/src/app/pages/AgentChat/parsing/toolLabels.ts @@ -267,7 +267,7 @@ const VARIANTS: Record = { ], }; -// Keys match backend _sanitize_server_name in tools_lib. +// Keys match backend sanitize_mcp_server_name in tools_lib. const MCP_SERVER_BRAND: Record = { 'google-workspace': 'Google Workspace', 'microsoft-365': 'Microsoft 365', diff --git a/linter/README.md b/linter/README.md index 98e06df1..c66c9aff 100644 --- a/linter/README.md +++ b/linter/README.md @@ -12,7 +12,9 @@ This folder contains the project's code quality tooling: a structural linter, de **Unused Python code (Vulture)** — Flags unused functions, classes, variables, and imports in the backend. Integrated into the linter's watch loop — findings appear as warnings in the Problems panel alongside structural errors. Confidence thresholds are configurable via `vulture-min-confidence` and `vulture-error-threshold`. -These rules apply to `.py`, `.ts`, `.tsx`, `.js`, and `.jsx` files. +**No leading-underscore names (`no-underscore-names`)** — Bans names that start with `_` in backend Python: functions, methods, arguments, classes, variable bindings, instance/class attribute writes (`self._x = ...`), and import aliases. The prefix is a blind spot — Pylance's `reportUnusedVariable`, ruff's `dummy-variable-rgx` (`F841`/`ARG0xx`), and vulture all treat a leading underscore as "intentionally private/unused" and stop reporting it, so dead `_name` code slips through every dead-code tool at once. Dunders (`__init__`, `__repr__`, … — required by Python) and the bare `_` throwaway (`for _ in …`) are exempt; name-mangled `__x` is **not**. The rule ships strict with no exceptions seeded — offenders are renamed by hand. + +These rules apply to `.py`, `.ts`, `.tsx`, `.js`, and `.jsx` files (the `no-underscore-names` rule is Python-only). ### Orphaned endpoints @@ -82,6 +84,7 @@ Or use the `knip:check` VS Code task (`Cmd+Shift+P` → "Run Task" → "knip:che "max-folder-items": true, "no-nested-imports": true, "vulture": true, + "no-underscore-names": true, "eslint": true, "knip": true, "endpoints": true, @@ -113,6 +116,23 @@ Set any key in `"enabled"` to `false` to skip that check entirely. Missing keys `config/vulture_whitelist.py` suppresses false positives — symbols used by frameworks, entry points, or external consumers that vulture can't detect statically. Add bare names to the file to mark them as intentionally used. +### Vulture inline ignores + +For one-off false positives where the suppression reads best next to the code, drop a comment on the flagged line. Vulture points at the definition line (the `def`/`class` line, or the assignment line for an attribute), which is where the comment goes. + +| Comment | Effect | +|---------|--------| +| `# vulture-ignore` | Silences any vulture finding on that line | +| `# vulture-ignore: name1, name2` | Silences only when the finding names one of the listed symbols | + +The scoped form is preferred — it can't accidentally swallow an unrelated future finding on the same line. Example: + +```python +gauth.get_credentials = _patched_get_credentials # vulture-ignore: get_credentials +``` + +Prefer the whitelist for symbols exempt across many call sites; prefer an inline ignore when the exemption is local and benefits from sitting next to the code. + ### ESLint `frontend/eslint.config.mjs` — flat config format (ESLint v9). The key rule for unused code is `@typescript-eslint/no-unused-vars`. Prefix a variable with `_` to suppress the warning. @@ -155,6 +175,7 @@ linter/ checks/ # check implementations __init__.py # shared filter/match utilities + .lintignore support structural.py # file length, folder size, nested imports + no_underscore_names.py # bans leading-underscore Python names vulture.py # vulture dead-code runner eslint.py # eslint runner knip.py # knip unused-code runner @@ -181,6 +202,7 @@ deferred. | `max-file-lines` (300) | on | Our 300-line precedence. Active for new files; existing debt is grandfathered (see below). | | `max-folder-items` (7) | on | Grandfathered per subtree via `.lintignore-max-folder-items` markers in `backend/`, `frontend/`, `debugger/`, `electron/`, `scripts/`. | | `vulture` | on | Dead-code detection over `backend/`. Runs against `backend/.venv/bin/vulture`. | +| `no-underscore-names` | on | Bans leading-underscore Python names over `backend/`. Pure-AST, no external tool. Strict with no seeded exceptions — existing offenders are renamed by hand. | | `no-nested-imports` | off | We deliberately use function-level / lazy imports to break import cycles (400+ sites). Flagging them all is wrong for this codebase. | | `eslint`, `knip` | off | Node tooling, deferred to a later pass. | | `endpoints` | off | Orphaned-endpoint triage deferred. | diff --git a/linter/checks/_models.py b/linter/checks/_models.py new file mode 100644 index 00000000..fb23da27 --- /dev/null +++ b/linter/checks/_models.py @@ -0,0 +1,81 @@ +"""Shared model metadata for the linter. + +Pydantic ``BaseModel`` field names are collected here so independent checks can +agree on which attribute names belong to a serialization schema, and are +therefore "used" even when the only *read* happens across a language boundary +(serialized to JSON over the wire and consumed by the frontend) which vulture's +Python-only analysis structurally cannot see. + +Framework detection lives in ONE place so checks/classes.py and +checks/vulture.py never drift on what counts as a model. This module owns no +section of its own and is not gated by config "enabled" flags, so any check that +imports it works regardless of which sections are turned on. +""" + +from __future__ import annotations + +import ast +from functools import lru_cache +from pathlib import Path + +FRAMEWORK_BASES = {"BaseModel"} + + +def is_framework_model(cls: ast.ClassDef) -> bool: + """True when *cls* subclasses a known framework base (e.g. pydantic BaseModel).""" + return any( + (isinstance(b, ast.Name) and b.id in FRAMEWORK_BASES) + or (isinstance(b, ast.Attribute) and b.attr in FRAMEWORK_BASES) + for b in cls.bases + ) + + +@lru_cache(maxsize=512) +def _fields_in_file_cached(filepath: str, _mtime: float) -> frozenset[str]: + """Annotated field names on framework models in *filepath*. + + Keyed on *(filepath, mtime)* so the long-lived watch process re-parses a file + after it is edited instead of returning a stale set from an earlier version. + """ + try: + tree = ast.parse(Path(filepath).read_text()) + except (OSError, SyntaxError): + return frozenset() + names: set[str] = set() + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef) or not is_framework_model(node): + continue + # Direct body only: pydantic fields are annotated assignments + # (``name: type`` / ``name: type = default``). Nested models are picked + # up on their own ClassDef pass by ast.walk. + for stmt in node.body: + if isinstance(stmt, ast.AnnAssign) and isinstance(stmt.target, ast.Name): + names.add(stmt.target.id) + return frozenset(names) + + +def _fields_in_file(filepath: Path) -> frozenset[str]: + try: + mtime = filepath.stat().st_mtime + except OSError: + return frozenset() + return _fields_in_file_cached(str(filepath), mtime) + + +def collect_pydantic_field_names(root: Path) -> set[str]: + """Every annotated field name on a ``BaseModel`` subclass under ``backend/``. + + Consumed by checks/vulture.py to treat ``obj. = ...`` writes as live + even when the only read is across the Python/TS boundary, which vulture would + otherwise report as an unused attribute. + """ + backend = root / "backend" + if not backend.is_dir(): + return set() + names: set[str] = set() + for pyfile in backend.rglob("*.py"): + parts = pyfile.parts + if ".venv" in parts or "__pycache__" in parts: + continue + names |= _fields_in_file(pyfile) + return names diff --git a/linter/checks/classes.py b/linter/checks/classes.py index 38e0bd04..236a26fe 100644 --- a/linter/checks/classes.py +++ b/linter/checks/classes.py @@ -12,16 +12,7 @@ import ast from pathlib import Path from . import is_excepted, is_excluded, is_lintignored - -FRAMEWORK_BASES = {"BaseModel"} - - -def _is_framework_model(cls: ast.ClassDef) -> bool: - return any( - (isinstance(b, ast.Name) and b.id in FRAMEWORK_BASES) - or (isinstance(b, ast.Attribute) and b.attr in FRAMEWORK_BASES) - for b in cls.bases - ) +from ._models import is_framework_model def run_class_check( @@ -53,7 +44,7 @@ def run_class_check( for node in ast.walk(tree): if not isinstance(node, ast.ClassDef): continue - if _is_framework_model(node): + if is_framework_model(node): continue # Tier 2 placeholder: non-framework classes are skipped until # cross-reference analysis is implemented. diff --git a/linter/checks/no_underscore_names.py b/linter/checks/no_underscore_names.py new file mode 100644 index 00000000..f4097068 --- /dev/null +++ b/linter/checks/no_underscore_names.py @@ -0,0 +1,148 @@ +"""Ban leading-underscore names that dead-code tooling silently skips. + +Pylance's reportUnusedVariable, ruff's dummy-variable-rgx (F841/ARG0xx), and +vulture all treat a leading underscore as "intentionally private/unused" and +stop reporting it -- which makes ``_name`` a blind spot for dead-code detection. +This check bans the prefix so nothing can hide behind it. + +Exempt: dunders (``__init__`` and friends, i.e. ``__x__``) and the bare ``_`` +throwaway (``for _ in ...`` / ``a, _ = unpack()``). Everything else starting +with ``_`` is flagged, including name-mangled ``__x``. + +Covers function/method names, arguments (incl. lambda), class names, variable +bindings (assignments, annotations, walrus, loop/with/except targets, tuple +unpacking), instance/class attribute writes (``self._x = ...``), and import +aliases. Scoped to ``backend/`` Python, like checks/classes.py. +""" + +from __future__ import annotations + +import ast +from collections.abc import Callable +from pathlib import Path + +from . import is_excepted, is_excluded, is_lintignored + +RULE = "no-underscore-names" + +Report = Callable[[str, int, int, str], None] + + +def _is_dunder(name: str) -> bool: + """True for ``__x__`` style names that Python requires (``__init__`` etc.).""" + return len(name) > 4 and name.startswith("__") and name.endswith("__") + + +def _flagged(name: str) -> bool: + if name == "_" or not name.startswith("_"): + return False + return not _is_dunder(name) + + +def _report_targets(target: ast.AST, report: Report) -> None: + """Walk an assignment/loop target down to the names it binds.""" + if isinstance(target, ast.Name): + report(target.id, target.lineno, target.col_offset, "variable") + elif isinstance(target, ast.Attribute): + # self._x = ... / cls._x = ...: the bound name is the attribute itself. + report(target.attr, target.end_lineno or target.lineno, _attr_col(target), "attribute") + elif isinstance(target, ast.Starred): + _report_targets(target.value, report) + elif isinstance(target, (ast.Tuple, ast.List)): + for elt in target.elts: + _report_targets(elt, report) + # ast.Subscript (``d["_x"] = ...``) is not a name binding and is skipped. + + +def _attr_col(node: ast.Attribute) -> int: + """Best-effort column for the attribute name (after the dot).""" + # value.end_col_offset points just past ``value``; +1 skips the dot. + end = getattr(node.value, "end_col_offset", None) + return (end + 1) if end is not None else node.col_offset + + +def _args(a: ast.arguments) -> list[ast.arg | None]: + return [*a.posonlyargs, *a.args, *a.kwonlyargs, a.vararg, a.kwarg] + + +def _check_tree(tree: ast.AST, rel: str) -> list[str]: + errors: list[str] = [] + seen: set[tuple[int, int, str]] = set() + + def report(name: str, lineno: int, col: int, kind: str) -> None: + if not _flagged(name): + return + key = (lineno, col, name) + if key in seen: + return + seen.add(key) + errors.append( + f"{rel}:{lineno}:{col + 1}: error: " + f"[{RULE}] {kind} '{name}' has a leading underscore" + ) + + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + report(node.name, node.lineno, node.col_offset, "function") + for arg in _args(node.args): + if arg is not None: + report(arg.arg, arg.lineno, arg.col_offset, "argument") + elif isinstance(node, ast.Lambda): + for arg in _args(node.args): + if arg is not None: + report(arg.arg, arg.lineno, arg.col_offset, "argument") + elif isinstance(node, ast.ClassDef): + report(node.name, node.lineno, node.col_offset, "class") + elif isinstance(node, (ast.Assign, ast.AnnAssign, ast.NamedExpr)): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + for t in targets: + _report_targets(t, report) + elif isinstance(node, (ast.For, ast.AsyncFor, ast.comprehension)): + _report_targets(node.target, report) + elif isinstance(node, (ast.With, ast.AsyncWith)): + for item in node.items: + if item.optional_vars is not None: + _report_targets(item.optional_vars, report) + elif isinstance(node, ast.ExceptHandler) and node.name: + report(node.name, node.lineno, node.col_offset, "exception") + elif isinstance(node, ast.Import): + for alias in node.names: + report( + alias.asname or alias.name.split(".")[0], + node.lineno, node.col_offset, "import", + ) + elif isinstance(node, ast.ImportFrom): + for alias in node.names: + if alias.name != "*": + report(alias.asname or alias.name, node.lineno, node.col_offset, "import") + + return errors + + +def run_underscore_check( + root: Path, + exceptions: dict[str, list[str]], + excludes: list[str], + ignores: dict[Path, set[str]] | None = None, +) -> list[str]: + """Flag leading-underscore names in backend Python files.""" + errors: list[str] = [] + backend = root / "backend" + if not backend.is_dir(): + return errors + + for pyfile in sorted(backend.rglob("*.py")): + if is_excluded(pyfile, root, excludes): + continue + rel = str(pyfile.relative_to(root)) + if is_excepted(rel, RULE, exceptions): + continue + if ignores and is_lintignored(pyfile, root, RULE, ignores): + continue + try: + tree = ast.parse(pyfile.read_text(), filename=rel) + except (OSError, SyntaxError): + continue + errors.extend(_check_tree(tree, rel)) + + return errors diff --git a/linter/checks/vulture.py b/linter/checks/vulture.py index a17b0919..b66bd2cc 100644 --- a/linter/checks/vulture.py +++ b/linter/checks/vulture.py @@ -20,6 +20,7 @@ from functools import lru_cache from pathlib import Path from . import CheckError, is_excepted, is_lintignored +from ._models import collect_pydantic_field_names CONFIG_DIR = Path(__file__).resolve().parent.parent / "config" @@ -64,6 +65,49 @@ def _is_inside_class(filepath: str, lineno: int) -> bool: return any(start < lineno <= end for start, end in _class_line_ranges(filepath)) +# Inline suppression: `# vulture-ignore` (bare) silences any finding on the +# line; `# vulture-ignore: name1, name2` only silences when the finding names +# one of the listed symbols (the noqa-with-codes ergonomic). +_INLINE_IGNORE_RE = re.compile(r"#\s*vulture-ignore\b(?::\s*(?P.*))?") + + +@lru_cache(maxsize=256) +def _file_lines_cached(filepath: str, _mtime: float) -> tuple[str, ...]: + """Return the source lines of *filepath*, keyed on *(filepath, mtime)* so the + long-lived watch process re-reads a file after it is edited.""" + try: + return tuple(Path(filepath).read_text().splitlines()) + except OSError: + return () + + +def _inline_ignored(filepath: Path, lineno: int, message: str) -> bool: + """True when the flagged line carries a ``# vulture-ignore`` comment. + + Vulture points at the definition line (the ``def``/``class`` line, or the + assignment line for an attribute), which is exactly where the comment lives. + The scoped form only suppresses when the vulture message names a listed + symbol, so the comment cannot accidentally swallow an unrelated future + finding on the same line. + """ + try: + mtime = filepath.stat().st_mtime + except OSError: + return False + lines = _file_lines_cached(str(filepath), mtime) + if not (1 <= lineno <= len(lines)): + return False + m = _INLINE_IGNORE_RE.search(lines[lineno - 1]) + if not m: + return False + names = m.group("names") + if not names: + return True + wanted = {n.strip() for n in names.replace(",", " ").split() if n.strip()} + flagged = re.search(r"'([^']+)'", message) + return bool(flagged and flagged.group(1) in wanted) + + def run_vulture( root: Path, min_confidence: int, error_threshold: int, exceptions: dict[str, list[str]], @@ -107,6 +151,15 @@ def run_vulture( detail = result.stderr.strip()[:300] or "no output" raise CheckError(f"vulture exited with code {result.returncode}: {detail}") + # Pydantic field names assigned outside the class body (e.g. + # ``session.memory_recalled = True``) are reported by vulture as unused + # attributes because the only *read* is across the Python/TS boundary + # (serialized via model_dump and consumed by the frontend). Treat any + # attribute whose name is a known model field as live. Computed once per run + # and independent of the "classes" section toggle, so suppression holds even + # when that section is disabled. + model_fields = collect_pydantic_field_names(root) + errors: list[str] = [] for line in result.stdout.strip().splitlines(): m = re.match(r"^(.+):(\d+): (.+)$", line) @@ -118,10 +171,15 @@ def run_vulture( # never gets a vote. if re.search(r"unused (import|variable)", message): continue + attr_m = re.match(r"unused attribute '([^']+)'", message) + if attr_m and attr_m.group(1) in model_fields: + continue if is_excepted(filepath, "vulture", exceptions): continue if ignores and is_lintignored(root / filepath, root, "vulture", ignores): continue + if _inline_ignored(root / filepath, int(lineno), message): + continue if _is_inside_class(str(root / filepath), int(lineno)): continue conf = re.search(r"\((\d+)% confidence\)", message) diff --git a/linter/config/config.json b/linter/config/config.json index 9e794375..d72fce40 100644 --- a/linter/config/config.json +++ b/linter/config/config.json @@ -6,6 +6,7 @@ "no-nested-imports": false, "vulture": true, "ruff": true, + "no-underscore-names": false, "eslint": false, "knip": false, "endpoints": false, @@ -16,6 +17,7 @@ "eslint-knip": "Node tooling deferred to a later pass.", "classes": "Placeholder check, not wired up. endpoints: orphaned-endpoint triage deferred.", "ruff-vulture-split": "ruff owns per-file/per-scope checks (F401 unused imports, F811 redefinitions, F841 unused locals, ARG001/ARG002 unused args), which it does AST-accurately and which vulture either gets wrong (global name-set hides per-file dead imports) or rates as noisy 60% findings. vulture is narrowed in checks/vulture.py to whole-program reachability only (dead functions/methods/classes/attributes), the one thing ruff structurally cannot do since it never builds a cross-module symbol graph. F401 honors __all__ and the redundant-alias form (import x as x) for intentional re-exports; add one of those if F401 flags a deliberate re-export.", + "no-underscore-names": "Bans leading-underscore names in backend/ Python (functions, methods, args, classes, variables, instance/class attribute writes, import aliases). The prefix is a blind spot: Pylance reportUnusedVariable, ruff dummy-variable-rgx (F841/ARG0xx), and vulture all treat it as intentionally-private/unused and stop reporting, so dead _name code hides. Exempt: dunders (__x__, required by Python) and the bare _ throwaway. Name-mangled __x IS flagged. Intentionally strict with no exceptions seeded -- offenders are renamed by hand.", "max-file-lines-exceptions": "Grandfather list of pre-existing >300-line files (existing debt, not new). Paths updated after the folder-tree restructure moved several of them.", "max-folder-items-exceptions": "Exact-path allow for folders intentionally over the cap. The rule trips at >7 (7 items is fine, the 8th tips it), so only genuinely 8+ folders are listed. backend/ and backend/apps are FastAPI feature-package registries (each child is an app mounted in main.py); agents/ aggregates agent subsystems; core/, tools_lib/, tests/ are conventionally flat. Frontend: app/pages is the page registry, AgentChat/ChatInput/Settings-sections/Onboarding are organizational parents, and shared/state (Redux slices) plus hooks/steps/mcp-cards/Views are flat peer collections. scripts/, electron/, linter/checks/ are flat tool dirs. These replaced blanket .lintignore-max-folder-items sentinels (backend, frontend, scripts, electron, linter/checks) so the rule still catches NEW unplanned bloat everywhere else. Kept as whole-subtree sentinels on purpose: debugger/ (self-contained injected sub-tool with its own Vite GUI), webapp_template (Vite scaffold payload), and vendored mcp-bundles.", "import-cycles": "Flags RUNTIME circular imports only (SCC>1). Skips type-only imports (import type / export type) and dynamic import() since neither runs at module init, which is why the idiomatic Redux store<->hooks type cycle is not flagged. Frontend alias resolution comes from import-cycle-aliases. Zero cycles today; the check keeps it that way." @@ -63,6 +65,7 @@ "import-cycles": [], "vulture": [], "ruff": [], + "no-underscore-names": [], "endpoints": [], "classes": [] } diff --git a/linter/lint.py b/linter/lint.py index 9c27ffe7..7b7fd810 100644 --- a/linter/lint.py +++ b/linter/lint.py @@ -20,6 +20,7 @@ from checks.knip import run_knip from checks.endpoints import run_endpoint_check from checks.classes import run_class_check from checks.cycles import run_cycle_check +from checks.no_underscore_names import run_underscore_check from watchfiles import watch, DefaultFilter SCRIPT_DIR = Path(__file__).resolve().parent @@ -27,7 +28,7 @@ CONFIG_FILE = SCRIPT_DIR / "config" / "config.json" # Print order for the sections; also the order they run in. SECTION_ORDER = [ - "structural", "vulture", "ruff", "eslint", + "structural", "vulture", "ruff", "no-underscore-names", "eslint", "knip", "endpoints", "classes", "import-cycles", ] @@ -152,6 +153,9 @@ def run_checks(root: Path) -> LintResult: run_section("ruff", lambda: run_ruff( root, rules.get("ruff-select", "F401,F811,F841,ARG001,ARG002"), exceptions, ignores, ) if enabled.get("ruff", True) else []) + run_section("no-underscore-names", lambda: run_underscore_check( + root, exceptions, excludes, ignores, + ) if enabled.get("no-underscore-names", True) else []) run_section("eslint", lambda: run_eslint(root, ignores) if enabled.get("eslint", True) else []) run_section("knip", lambda: run_knip(root, ignores) if enabled.get("knip", True) else []) run_section("endpoints", lambda: run_endpoint_check(