feat(ci): public-surface diff vs base (dropped public names, methods, test defs), advisory on PRs
The Sep 2026 whole-codebase refactor (PR #102117) opened with 1,703 public top-level names dropped across 341 modules, 1,000 public/dunder methods in 166, and 126 `def test_` deleted in 52 files. Reviewers found ~30 of the names by hand; the rest surfaced as post-merge rework: 10 commits restoring symbols and facade re-exports, 6 restoring tests, and a qwen OAuth break that passed import smoke because the caller used `module.attr`. Every one was catchable in seconds; nothing ran the check because it did not exist. scripts/ci/check_public_surface.py: AST diff of modules present on both sides of merge-base..HEAD. Public top-level names (defs, classes, assignments, imported/re-exported names), public and dunder methods of top-level classes, and `def test_` counts per tests/ file. Deleted modules and deleted test files are visible decisions and are not flagged; private names are not flagged. Advisory (exit 0, prints the report) by default; --strict exits 1 so a refactor brief or a CI lane can gate on it. Wired into lint.yml as an advisory PR step next to the compat-pointer check. Replayed on the refactor PR at open (63279301bcb..022785a541) it reports exactly the figures above in 18 s; on this branch vs main it reports 0. Test: a throwaway git repo with drops, private drops, a move-with-re-export, a lost test def and a changed non-source module; asserts the exact report and the advisory/strict exit codes.
This commit is contained in:
9
.github/workflows/lint.yml
vendored
9
.github/workflows/lint.yml
vendored
@@ -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
|
||||
|
||||
159
scripts/ci/check_public_surface.py
Normal file
159
scripts/ci/check_public_surface.py
Normal file
@@ -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())
|
||||
72
tests/scripts/test_check_public_surface.py
Normal file
72
tests/scripts/test_check_public_surface.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user