diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index bae4299b1b..633ea9ca66 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -184,3 +184,12 @@ jobs: # reverting one commit, so in-tree code must never depend on them. - name: Forbid in-tree use of plugin-compat pointers run: python scripts/check_compat_pointers.py + + # Advisory: dropped public names / methods / test defs vs the PR base, printed into the log. + # A refactor that silently removes a public symbol breaks plugins that import it; the Sep 2026 + # decomposition opened with 1,703 such drops that reviewers had to find by hand. + - name: Public-surface diff vs base (advisory) + if: github.event_name == 'pull_request' + run: | + git fetch --no-tags --depth=1 origin "${{ github.base_ref }}" + python scripts/ci/check_public_surface.py --base "origin/${{ github.base_ref }}" --head HEAD diff --git a/scripts/ci/check_public_surface.py b/scripts/ci/check_public_surface.py new file mode 100644 index 0000000000..f878e9dbd4 --- /dev/null +++ b/scripts/ci/check_public_surface.py @@ -0,0 +1,159 @@ +#!/usr/bin/env python3 +"""Public-surface diff against a base ref: dropped public names and dropped test functions. + +The Sep 2026 whole-codebase refactor (PR #102117) opened with 1,703 public top-level names dropped +across 341 modules, 1,000 public methods in 166, and 130 ``def test_`` deleted in 54 files. +Reviewers found ~30 of the names by hand; the rest surfaced as post-merge fixes (10 commits +restoring symbols/re-exports, 6 restoring tests, plus a qwen OAuth break that went past import +smoke because the caller used ``module.attr``). Every one of those was catchable in seconds with +this script; nothing ran it because it did not exist. + +Rules (deliberately narrow, no allowlist file to maintain): +- A PUBLIC top-level name (function, class, module-level assignment, re-exported import) or a + public/dunder method of a module present on BOTH sides may be moved and re-exported or + deprecated, never silently removed. A deleted MODULE is not a drop here (that is a visible + decision; ``check_doc_paths`` territory). +- A ``tests/**`` file present on both sides must not end with fewer ``def test_`` than it started + with. Deleting a whole test file is, again, a visible decision and not flagged. + +Advisory by default (prints the report, exit 0). ``--strict`` exits 1 on any finding so a +refactor brief or a CI lane can make it a hard gate. Runs in ~30 s over the whole tree. + +Usage: + python scripts/ci/check_public_surface.py [--base origin/main] [--head HEAD] [--strict] [--json out.json] +""" +from __future__ import annotations + +import argparse +import ast +import json +import re +import subprocess +import sys + +SRC_DIRS = ("agent", "gateway", "hermes_cli", "tools", "tui_gateway", "cron", "acp_adapter", "plugins") +_TEST_DEF_RE = re.compile(r"^\s*(?:async\s+)?def test_", re.M) + + +def _git(*args: str) -> str: + return subprocess.run(["git", *args], capture_output=True, text=True, encoding="utf-8", errors="replace").stdout + + +def _changed_py(base: str, head: str) -> list[str]: + out = _git("diff", "--name-only", "--diff-filter=M", base, head) + return [f for f in out.split() if f.endswith(".py")] + + +def _show(rev: str, path: str) -> str | None: + r = subprocess.run(["git", "show", f"{rev}:{path}"], capture_output=True, text=True, encoding="utf-8", errors="replace") + return r.stdout if r.returncode == 0 else None + + +def public_toplevel(src: str) -> set[str] | None: + """Public top-level names: defs, classes, assigned names, and imported/re-exported names.""" + try: + tree = ast.parse(src) + except SyntaxError: + return None + names: set[str] = set() + for node in tree.body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + names.add(node.name) + elif isinstance(node, ast.Assign): + for target in node.targets: + names.update(x.id for x in ast.walk(target) if isinstance(x, ast.Name)) + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + names.add(node.target.id) + elif isinstance(node, (ast.Import, ast.ImportFrom)): + names.update((a.asname or a.name).split(".")[0] for a in node.names) + return {n for n in names if not n.startswith("_")} + + +def public_methods(src: str) -> set[str]: + """``Class.method`` for public and dunder methods of top-level classes.""" + try: + tree = ast.parse(src) + except SyntaxError: + return set() + out: set[str] = set() + for node in tree.body: + if isinstance(node, ast.ClassDef): + for m in node.body: + if isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef)) and ( + not m.name.startswith("_") or m.name.startswith("__") + ): + out.add(f"{node.name}.{m.name}") + return out + + +def _is_source(path: str) -> bool: + return "/" not in path or path.split("/", 1)[0] in SRC_DIRS + + +def diff_surface(base: str, head: str) -> dict: + dropped_names: dict[str, list[str]] = {} + dropped_methods: dict[str, list[str]] = {} + test_drops: dict[str, tuple[int, int]] = {} + for path in _changed_py(base, head): + before, after = _show(base, path), _show(head, path) + if before is None or after is None: + continue + if path.startswith("tests/"): + nb, na = len(_TEST_DEF_RE.findall(before)), len(_TEST_DEF_RE.findall(after)) + if na < nb: + test_drops[path] = (nb, na) + continue + if not _is_source(path) or path.startswith("tests"): + continue + pb, pa = public_toplevel(before), public_toplevel(after) + if pb is not None and pa is not None and (gone := sorted(pb - pa)): + dropped_names[path] = gone + if gone_m := sorted(public_methods(before) - public_methods(after)): + dropped_methods[path] = gone_m + return {"dropped_names": dropped_names, "dropped_methods": dropped_methods, "test_drops": test_drops} + + +def render(report: dict) -> str: + lines: list[str] = [] + n_names = sum(len(v) for v in report["dropped_names"].values()) + n_meth = sum(len(v) for v in report["dropped_methods"].values()) + n_tests = sum(b - a for b, a in report["test_drops"].values()) + lines.append( + f"public-surface: {n_names} public top-level name(s) dropped in {len(report['dropped_names'])} module(s); " + f"{n_meth} public/dunder method(s) dropped in {len(report['dropped_methods'])}; " + f"{n_tests} test def(s) dropped in {len(report['test_drops'])} file(s)" + ) + for path, names in sorted(report["dropped_names"].items()): + lines.append(f" {path}: -{', '.join(names[:12])}{' …' if len(names) > 12 else ''}") + for path, names in sorted(report["dropped_methods"].items()): + lines.append(f" {path}: -{', '.join(names[:8])}{' …' if len(names) > 8 else ''}") + for path, (b, a) in sorted(report["test_drops"].items()): + lines.append(f" {path}: {b} -> {a} test defs") + if n_names or n_meth or n_tests: + lines.append( + " A public name may be moved and re-exported or deprecated, never silently removed (in-tree refs say " + "nothing about plugins); a test file must not lose test defs unless the commit names the private symbol " + "they pinned." + ) + return "\n".join(lines) + + +def main(argv: list[str] | None = None) -> int: + ap = argparse.ArgumentParser(description=(__doc__ or "").split("\n\n")[0]) + ap.add_argument("--base", default="origin/main") + ap.add_argument("--head", default="HEAD") + ap.add_argument("--strict", action="store_true", help="exit 1 on any finding") + ap.add_argument("--json", dest="json_out", help="also write the report as JSON") + args = ap.parse_args(argv) + base = _git("merge-base", args.base, args.head).strip() or args.base + report = diff_surface(base, args.head) + print(render(report)) + if args.json_out: + with open(args.json_out, "w", encoding="utf-8") as fh: + json.dump(report, fh, indent=1) + findings = any(report[k] for k in ("dropped_names", "dropped_methods", "test_drops")) + return 1 if (args.strict and findings) else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/scripts/test_check_public_surface.py b/tests/scripts/test_check_public_surface.py new file mode 100644 index 0000000000..e0e5f038f2 --- /dev/null +++ b/tests/scripts/test_check_public_surface.py @@ -0,0 +1,72 @@ +"""scripts/ci/check_public_surface.py detects silently dropped public names, methods and test defs. + +Replayed against the Sep 2026 refactor PR at open it reports 1,703 public names dropped in 341 modules +(the figure reviewers had to find ~30 of by hand); here the contract is pinned on a throwaway repo. +""" +import importlib.util +import subprocess +import textwrap +from pathlib import Path + +SCRIPT = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "check_public_surface.py" + + +def _load(): + spec = importlib.util.spec_from_file_location("check_public_surface", SCRIPT) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def _git(repo, *args): + subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True, + env={"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", "PATH": "/usr/bin:/bin:/usr/local/bin"}) + + +def test_dropped_public_names_methods_and_test_defs_are_reported_and_moves_are_not(tmp_path, monkeypatch): + repo = tmp_path + (repo / "agent").mkdir(); (repo / "tests").mkdir(); (repo / "docs").mkdir() + (repo / "agent" / "mod.py").write_text(textwrap.dedent(''' + import os + from json import loads as parse + + LIMIT = 3 + def keep(): ... + def gone(): ... + def _private(): ... + class K: + def pub(self): ... + def __len__(self): return 0 + def _hidden(self): ... + '''), encoding="utf-8") + (repo / "agent" / "moved.py").write_text("def relocated(): ...\n", encoding="utf-8") + (repo / "tests" / "test_x.py").write_text("def test_a(): ...\ndef test_b(): ...\nasync def test_c(): ...\n", encoding="utf-8") + (repo / "docs" / "notes.py").write_text("def doc_helper(): ...\n", encoding="utf-8") + _git(repo, "init", "-q", "-b", "main"); _git(repo, "add", "."); _git(repo, "commit", "-qm", "base") + + # HEAD: drop `gone`, `parse`, `K.pub`, `K.__len__`; drop `_private`/`K._hidden` (private: fine); + # move `relocated` into mod with a re-export left behind (fine); lose one test def; change a + # non-source module (ignored). + (repo / "agent" / "mod.py").write_text(textwrap.dedent(''' + import os + from agent.moved import relocated # re-export + + LIMIT = 3 + def keep(): ... + class K: ... + '''), encoding="utf-8") + (repo / "agent" / "moved.py").write_text("def relocated(): ...\ndef relocated2(): ...\n", encoding="utf-8") + (repo / "tests" / "test_x.py").write_text("def test_a(): ...\nasync def test_c(): ...\n", encoding="utf-8") + (repo / "docs" / "notes.py").write_text("def other(): ...\n", encoding="utf-8") + _git(repo, "add", "."); _git(repo, "commit", "-qm", "head") + + monkeypatch.chdir(repo) + mod = _load() + report = mod.diff_surface("HEAD~1", "HEAD") + assert report["dropped_names"] == {"agent/mod.py": ["gone", "parse"]} + assert report["dropped_methods"] == {"agent/mod.py": ["K.__len__", "K.pub"]} + assert report["test_drops"] == {"tests/test_x.py": (3, 2)} + assert mod.main(["--base", "HEAD~1", "--head", "HEAD"]) == 0 # advisory + assert mod.main(["--base", "HEAD~1", "--head", "HEAD", "--strict"]) == 1 + assert mod.main(["--base", "HEAD", "--head", "HEAD", "--strict"]) == 0 # nothing dropped