mirror of
https://github.com/openswarm-ai/openswarm.git
synced 2026-09-11 12:17:45 +02:00
[haik]: add p-private linter check enforcing the p_ prefix as a real access modifier with Java private semantics over backend Python — module-level p_ symbols (top-level def/class/assignment) are file-private (usable in their own file, nowhere else), class members (def p_m, self.p_x=, class-body fields p_x: T) are class-private (legal only lexically inside the owning class or a class nested within it); strict like Java private not protected so cross-file subclass access to a base p_ member is flagged; two-pass pure-AST implementation in checks/p_private.py (phase 1 collects ownership, phase 2 checks references via access form — attribute access for class members, bare-name/import for module-level — no type inference); wire run_p_private_check into lint.py section order between no-underscore-names and eslint, add p-private config entries (enabled, description, empty exceptions) in config.json, and document the rule and its status in README
This commit is contained in:
@@ -14,6 +14,8 @@ This folder contains the project's code quality tooling: a structural linter, de
|
||||
|
||||
**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.
|
||||
|
||||
**`p_` private convention (`p-private`)** — Enforces the `p_` prefix as a real access modifier with **Java `private` semantics** over backend Python. Module-level `p_` symbols (top-level `def`/`class`/assignment) are **file-private** — usable anywhere in their own file, nowhere else. Class members (`def p_m`, `self.p_x = …`, class-body fields `p_x: T`) are **class-private** — a `recv.p_x` access is legal only lexically inside the owning class (or a class nested within it). Strict like Java `private`, not `protected`: a subclass in another file reaching a base's `p_` member is flagged. Nested/inner classes may reach the enclosing class's members (the whole enclosing-class stack is checked). Detection is access-form based — attribute access governs class members, bare-name/import governs module-level — so no type inference is needed. No exemptions: tests and `__init__.py` re-exports are enforced. Greenfield today (zero `p_` in the tree), so it ships green and only fires once the convention is used.
|
||||
|
||||
These rules apply to `.py`, `.ts`, `.tsx`, `.js`, and `.jsx` files (the `no-underscore-names` rule is Python-only).
|
||||
|
||||
### Orphaned endpoints
|
||||
@@ -85,6 +87,7 @@ Or use the `knip:check` VS Code task (`Cmd+Shift+P` → "Run Task" → "knip:che
|
||||
"no-nested-imports": true,
|
||||
"vulture": true,
|
||||
"no-underscore-names": true,
|
||||
"p-private": true,
|
||||
"eslint": true,
|
||||
"knip": true,
|
||||
"endpoints": true,
|
||||
@@ -176,6 +179,7 @@ linter/
|
||||
__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
|
||||
p_private.py # enforces p_ private convention (Java-private scoping)
|
||||
vulture.py # vulture dead-code runner
|
||||
eslint.py # eslint runner
|
||||
knip.py # knip unused-code runner
|
||||
@@ -203,6 +207,7 @@ deferred.
|
||||
| `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. |
|
||||
| `p-private` | on | Enforces the `p_` private convention (Java `private` scoping: module-level = file-private, class members = class-private) over `backend/`. Pure-AST two-pass, no external tool. Greenfield — zero findings today. |
|
||||
| `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. |
|
||||
|
||||
@@ -0,0 +1,207 @@
|
||||
"""Cross-file/class privacy for the ``p_`` naming convention (Java ``private``).
|
||||
|
||||
A name prefixed with ``p_`` is private to the scope that owns it, enforced the
|
||||
way Java enforces ``private``:
|
||||
|
||||
* **Module-level** ``p_`` symbols (top-level ``def`` / ``class`` / assignment)
|
||||
are **file-private** -- usable anywhere in their own file, nowhere else.
|
||||
* **Class members** (``def p_m``, ``self.p_x = ...``, and class-body fields
|
||||
``p_x: T``) are **class-private** -- any ``recv.p_x`` is legal only when it
|
||||
appears lexically inside the owning class (or a class nested within it),
|
||||
matching Java's type-scoped ``private`` while side-stepping type inference.
|
||||
|
||||
Strict: a subclass in another file reaching a base's ``p_`` member is a
|
||||
violation (Java ``private``, not ``protected``). Nested/inner classes may reach
|
||||
the enclosing class's members and vice versa (the whole enclosing-class stack is
|
||||
checked). No exemptions -- tests and ``__init__.py`` re-exports are enforced.
|
||||
|
||||
How access is detected (no type inference needed): Python reaches class members
|
||||
only through attribute access (``self.p_x`` / ``obj.p_x``) and module-level
|
||||
symbols only by bare name (legal in-file) or ``module.p_x`` / ``from m import
|
||||
p_x`` across files. So the access *form* selects the scoping rule:
|
||||
|
||||
* ``recv.p_x`` (attribute load) -> class rule, else module rule
|
||||
* ``from m import p_x`` -> module rule (cross-file import = leak)
|
||||
|
||||
Two passes: phase 1 records ownership, phase 2 checks references against it.
|
||||
Scoped to ``backend/`` Python, like checks/classes.py.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
from . import is_excepted, is_excluded, is_lintignored
|
||||
|
||||
RULE = "p-private"
|
||||
|
||||
# name -> set of relative file paths that define it at module level
|
||||
ModuleOwners = dict[str, set[str]]
|
||||
# name -> set of (relative file path, qualified class name) that define it as a member
|
||||
ClassOwners = dict[str, set[tuple[str, str]]]
|
||||
|
||||
|
||||
def _is_p(name: str) -> bool:
|
||||
return len(name) > 2 and name.startswith("p_")
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Phase 1: ownership collection #
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
def _own(name: str, rel: str, stack: list[str], container: str, mp: ModuleOwners, cp: ClassOwners) -> None:
|
||||
if container == "class":
|
||||
cp.setdefault(name, set()).add((rel, stack[-1]))
|
||||
elif container == "module":
|
||||
mp.setdefault(name, set()).add(rel)
|
||||
# container == "function" -> local definition, not externally reachable
|
||||
|
||||
|
||||
def _own_target(t: ast.AST, rel: str, stack: list[str], container: str, mp: ModuleOwners, cp: ClassOwners) -> None:
|
||||
if isinstance(t, ast.Name) and _is_p(t.id):
|
||||
_own(t.id, rel, stack, container, mp, cp)
|
||||
elif isinstance(t, ast.Attribute) and _is_p(t.attr):
|
||||
# self._x = ... / cls._x = ...: owned by the lexically enclosing class.
|
||||
if stack:
|
||||
cp.setdefault(t.attr, set()).add((rel, stack[-1]))
|
||||
elif isinstance(t, ast.Starred):
|
||||
_own_target(t.value, rel, stack, container, mp, cp)
|
||||
elif isinstance(t, (ast.Tuple, ast.List)):
|
||||
for elt in t.elts:
|
||||
_own_target(elt, rel, stack, container, mp, cp)
|
||||
|
||||
|
||||
def _collect(node: ast.AST, rel: str, stack: list[str], container: str, mp: ModuleOwners, cp: ClassOwners) -> None:
|
||||
if isinstance(node, ast.ClassDef):
|
||||
if _is_p(node.name):
|
||||
_own(node.name, rel, stack, container, mp, cp)
|
||||
qual = f"{stack[-1]}.{node.name}" if stack else node.name
|
||||
for child in node.body:
|
||||
_collect(child, rel, [*stack, qual], "class", mp, cp)
|
||||
return
|
||||
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
||||
if _is_p(node.name):
|
||||
_own(node.name, rel, stack, container, mp, cp)
|
||||
for child in node.body:
|
||||
_collect(child, rel, stack, "function", mp, cp)
|
||||
return
|
||||
if isinstance(node, (ast.Assign, ast.AnnAssign)):
|
||||
targets = node.targets if isinstance(node, ast.Assign) else [node.target]
|
||||
for t in targets:
|
||||
_own_target(t, rel, stack, container, mp, cp)
|
||||
return
|
||||
# Plain compound statements (if/for/while/with/try/...) keep the container so
|
||||
# a conditionally-defined member is still attributed to its real scope.
|
||||
for child in ast.iter_child_nodes(node):
|
||||
_collect(child, rel, stack, container, mp, cp)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Phase 2: reference checking #
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
def _emit(out: list[str], seen: set[tuple[int, int, str]], rel: str, node: ast.AST, msg: str) -> None:
|
||||
key = (node.lineno, node.col_offset, msg)
|
||||
if key in seen:
|
||||
return
|
||||
seen.add(key)
|
||||
out.append(f"{rel}:{node.lineno}:{node.col_offset + 1}: error: [{RULE}] {msg}")
|
||||
|
||||
|
||||
def _check_attr(
|
||||
node: ast.Attribute, rel: str, stack: list[str],
|
||||
mp: ModuleOwners, cp: ClassOwners, out: list[str], seen: set[tuple[int, int, str]],
|
||||
) -> None:
|
||||
name = node.attr
|
||||
if name in cp:
|
||||
if cp[name] & {(rel, q) for q in stack}:
|
||||
return # lexically inside an owning class -> legal
|
||||
owners = ", ".join(sorted(f"{q} ({r})" for r, q in cp[name]))
|
||||
_emit(out, seen, rel, node, f"class-private '{name}' accessed outside its class (owner: {owners})")
|
||||
return
|
||||
if name in mp:
|
||||
if rel in mp[name]:
|
||||
return
|
||||
owners = ", ".join(sorted(mp[name]))
|
||||
_emit(out, seen, rel, node, f"module-private '{name}' accessed outside its file (defined in {owners})")
|
||||
|
||||
|
||||
def _check_import(
|
||||
name: str, node: ast.AST, rel: str,
|
||||
mp: ModuleOwners, out: list[str], seen: set[tuple[int, int, str]],
|
||||
) -> None:
|
||||
if name in mp and rel not in mp[name]:
|
||||
owners = ", ".join(sorted(mp[name]))
|
||||
_emit(out, seen, rel, node, f"module-private '{name}' imported outside its file (defined in {owners})")
|
||||
|
||||
|
||||
def _check_refs(
|
||||
node: ast.AST, rel: str, stack: list[str],
|
||||
mp: ModuleOwners, cp: ClassOwners, out: list[str], seen: set[tuple[int, int, str]],
|
||||
) -> None:
|
||||
if isinstance(node, ast.ClassDef):
|
||||
qual = f"{stack[-1]}.{node.name}" if stack else node.name
|
||||
# Decorators/bases are evaluated in the enclosing scope, not inside the class.
|
||||
for outer in (*node.decorator_list, *node.bases, *node.keywords):
|
||||
_check_refs(outer, rel, stack, mp, cp, out, seen)
|
||||
for child in node.body:
|
||||
_check_refs(child, rel, [*stack, qual], mp, cp, out, seen)
|
||||
return
|
||||
if isinstance(node, ast.Attribute) and isinstance(node.ctx, ast.Load) and _is_p(node.attr):
|
||||
_check_attr(node, rel, stack, mp, cp, out, seen)
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
for alias in node.names:
|
||||
if alias.name != "*" and _is_p(alias.name):
|
||||
_check_import(alias.name, node, rel, mp, out, seen)
|
||||
# A function does not open a new *class* scope, so the class stack is carried
|
||||
# through unchanged; that's why generic descent (not an early return) is right.
|
||||
for child in ast.iter_child_nodes(node):
|
||||
_check_refs(child, rel, stack, mp, cp, out, seen)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Runner #
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
||||
def run_p_private_check(
|
||||
root: Path,
|
||||
exceptions: dict[str, list[str]],
|
||||
excludes: list[str],
|
||||
ignores: dict[Path, set[str]] | None = None,
|
||||
) -> list[str]:
|
||||
"""Flag ``p_`` symbols accessed outside their owning file/class."""
|
||||
backend = root / "backend"
|
||||
if not backend.is_dir():
|
||||
return []
|
||||
|
||||
# Parse once; phase 1 must see *every* file (even exempted ones) so ownership
|
||||
# is known when a non-exempt file reaches into them.
|
||||
trees: list[tuple[Path, str, ast.AST]] = []
|
||||
mp: ModuleOwners = {}
|
||||
cp: ClassOwners = {}
|
||||
for pyfile in sorted(backend.rglob("*.py")):
|
||||
if is_excluded(pyfile, root, excludes):
|
||||
continue
|
||||
rel = str(pyfile.relative_to(root))
|
||||
try:
|
||||
tree = ast.parse(pyfile.read_text(), filename=rel)
|
||||
except (OSError, SyntaxError):
|
||||
continue
|
||||
trees.append((pyfile, rel, tree))
|
||||
for child in ast.iter_child_nodes(tree):
|
||||
_collect(child, rel, [], "module", mp, cp)
|
||||
|
||||
# Phase 2 reports only for files that aren't exempted/lintignored.
|
||||
errors: list[str] = []
|
||||
for pyfile, rel, tree in trees:
|
||||
if is_excepted(rel, RULE, exceptions):
|
||||
continue
|
||||
if ignores and is_lintignored(pyfile, root, RULE, ignores):
|
||||
continue
|
||||
seen: set[tuple[int, int, str]] = set()
|
||||
out: list[str] = []
|
||||
_check_refs(tree, rel, [], mp, cp, out, seen)
|
||||
errors.extend(out)
|
||||
|
||||
return errors
|
||||
@@ -7,6 +7,7 @@
|
||||
"vulture": true,
|
||||
"ruff": true,
|
||||
"no-underscore-names": false,
|
||||
"p-private": true,
|
||||
"eslint": false,
|
||||
"knip": false,
|
||||
"endpoints": false,
|
||||
@@ -18,6 +19,7 @@
|
||||
"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.",
|
||||
"p-private": "Enforces the p_ private convention with Java-private semantics over backend/ Python. Module-level p_ symbols (top-level def/class/assignment) are file-private; class members (def p_m, self.p_x=, class-body fields p_x: T) are class-private. A reference is legal only inside the owning file (module-level) or lexically inside the owning class / a class nested in it (members). Strict: cross-file subclass access to a base's p_ member is flagged (private, not protected). Nested classes may reach the enclosing class's members (whole enclosing-class stack is checked). Detection is access-form based (no type inference): attribute access governs class members, bare-name/import governs module-level. No exemptions -- tests and __init__.py re-exports are enforced. Greenfield today (zero p_ in tree), so it ships green and only fires once the convention is used.",
|
||||
"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."
|
||||
@@ -66,6 +68,7 @@
|
||||
"vulture": [],
|
||||
"ruff": [],
|
||||
"no-underscore-names": [],
|
||||
"p-private": [],
|
||||
"endpoints": [],
|
||||
"classes": []
|
||||
}
|
||||
|
||||
+5
-1
@@ -21,6 +21,7 @@ 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 checks.p_private import run_p_private_check
|
||||
from watchfiles import watch, DefaultFilter
|
||||
|
||||
SCRIPT_DIR = Path(__file__).resolve().parent
|
||||
@@ -28,7 +29,7 @@ CONFIG_FILE = SCRIPT_DIR / "config" / "config.json"
|
||||
|
||||
# Print order for the sections; also the order they run in.
|
||||
SECTION_ORDER = [
|
||||
"structural", "vulture", "ruff", "no-underscore-names", "eslint",
|
||||
"structural", "vulture", "ruff", "no-underscore-names", "p-private", "eslint",
|
||||
"knip", "endpoints", "classes", "import-cycles",
|
||||
]
|
||||
|
||||
@@ -156,6 +157,9 @@ def run_checks(root: Path) -> LintResult:
|
||||
run_section("no-underscore-names", lambda: run_underscore_check(
|
||||
root, exceptions, excludes, ignores,
|
||||
) if enabled.get("no-underscore-names", True) else [])
|
||||
run_section("p-private", lambda: run_p_private_check(
|
||||
root, exceptions, excludes, ignores,
|
||||
) if enabled.get("p-private", 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(
|
||||
|
||||
Reference in New Issue
Block a user