mirror of
https://github.com/rookiestar28/ComfyUI-OpenClaw.git
synced 2026-08-14 08:52:45 +00:00
chore(test): add exception boundary governance
This commit is contained in:
rookiestar28
+2
-2
@@ -249,11 +249,11 @@ class CommandRouter:
|
||||
if platform == "telegram":
|
||||
try:
|
||||
uid = int(sender_id)
|
||||
except Exception:
|
||||
except ValueError:
|
||||
uid = None
|
||||
try:
|
||||
cid = int(channel_id)
|
||||
except Exception:
|
||||
except ValueError:
|
||||
cid = None
|
||||
if uid is not None and uid in self.config.telegram_allowed_users:
|
||||
return True
|
||||
|
||||
@@ -0,0 +1,175 @@
|
||||
"""Verify selected broad-exception boundary policy.
|
||||
|
||||
This intentionally checks only modules listed in tests/exception_boundary_policy.json.
|
||||
The repo still has too many historical broad catches for a global BLE001-style
|
||||
rule to be useful.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import ast
|
||||
import json
|
||||
from collections import Counter
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import Any, Iterable
|
||||
|
||||
VALID_CLASSIFICATIONS = {
|
||||
"allowed_boundary_guard",
|
||||
"needs_narrowing",
|
||||
"needs_follow_up_test_coverage",
|
||||
}
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class BroadCatch:
|
||||
path: str
|
||||
line: int
|
||||
scope: str
|
||||
catch_type: str
|
||||
|
||||
|
||||
class _BroadCatchVisitor(ast.NodeVisitor):
|
||||
def __init__(self, path: Path):
|
||||
self.path = path.as_posix()
|
||||
self.scope_stack: list[str] = []
|
||||
self.catches: list[BroadCatch] = []
|
||||
|
||||
def visit_ClassDef(self, node: ast.ClassDef) -> None:
|
||||
self.scope_stack.append(node.name)
|
||||
self.generic_visit(node)
|
||||
self.scope_stack.pop()
|
||||
|
||||
def visit_FunctionDef(self, node: ast.FunctionDef) -> None:
|
||||
self.scope_stack.append(node.name)
|
||||
self.generic_visit(node)
|
||||
self.scope_stack.pop()
|
||||
|
||||
def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None:
|
||||
self.visit_FunctionDef(node)
|
||||
|
||||
def visit_ExceptHandler(self, node: ast.ExceptHandler) -> None:
|
||||
catch_type = _catch_type_name(node.type)
|
||||
if catch_type in {"bare", "Exception", "BaseException"}:
|
||||
self.catches.append(
|
||||
BroadCatch(
|
||||
path=self.path,
|
||||
line=node.lineno,
|
||||
scope=".".join(self.scope_stack) or "<module>",
|
||||
catch_type=catch_type,
|
||||
)
|
||||
)
|
||||
self.generic_visit(node)
|
||||
|
||||
|
||||
def _catch_type_name(node: ast.expr | None) -> str:
|
||||
if node is None:
|
||||
return "bare"
|
||||
if isinstance(node, ast.Name):
|
||||
return node.id
|
||||
if isinstance(node, ast.Tuple):
|
||||
names = {_catch_type_name(item) for item in node.elts}
|
||||
if "BaseException" in names:
|
||||
return "BaseException"
|
||||
if "Exception" in names:
|
||||
return "Exception"
|
||||
return ""
|
||||
|
||||
|
||||
def iter_broad_catches(path: Path) -> Iterable[BroadCatch]:
|
||||
tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path))
|
||||
visitor = _BroadCatchVisitor(path)
|
||||
visitor.visit(tree)
|
||||
return tuple(visitor.catches)
|
||||
|
||||
|
||||
def load_policy(path: Path) -> dict[str, Any]:
|
||||
return json.loads(path.read_text(encoding="utf-8"))
|
||||
|
||||
|
||||
def validate_exception_boundary_policy(
|
||||
repo_root: Path,
|
||||
policy: dict[str, Any],
|
||||
) -> list[str]:
|
||||
failures: list[str] = []
|
||||
modules = policy.get("selected_modules")
|
||||
if not isinstance(modules, dict) or not modules:
|
||||
return ["policy selected_modules must be a non-empty object"]
|
||||
|
||||
for rel_path, module_policy in sorted(modules.items()):
|
||||
path = repo_root / rel_path
|
||||
if not path.is_file():
|
||||
failures.append(f"{rel_path}: selected module does not exist")
|
||||
continue
|
||||
|
||||
allowed = module_policy.get("broad_catches")
|
||||
if not isinstance(allowed, list):
|
||||
failures.append(f"{rel_path}: broad_catches must be a list")
|
||||
continue
|
||||
|
||||
entries_by_scope: dict[str, dict[str, Any]] = {}
|
||||
for index, entry in enumerate(allowed):
|
||||
if not isinstance(entry, dict):
|
||||
failures.append(f"{rel_path}: broad_catches[{index}] must be an object")
|
||||
continue
|
||||
scope = entry.get("scope")
|
||||
classification = entry.get("classification")
|
||||
reason = entry.get("reason")
|
||||
if not isinstance(scope, str) or not scope:
|
||||
failures.append(f"{rel_path}: broad_catches[{index}] missing scope")
|
||||
continue
|
||||
if scope in entries_by_scope:
|
||||
failures.append(f"{rel_path}: duplicate broad-catch scope {scope}")
|
||||
entries_by_scope[scope] = entry
|
||||
if classification not in VALID_CLASSIFICATIONS:
|
||||
failures.append(
|
||||
f"{rel_path}:{scope}: invalid classification {classification!r}"
|
||||
)
|
||||
if not isinstance(reason, str) or not reason.strip():
|
||||
failures.append(f"{rel_path}:{scope}: missing reason")
|
||||
|
||||
catches = tuple(iter_broad_catches(path))
|
||||
counts = Counter(catch.scope for catch in catches)
|
||||
for catch in catches:
|
||||
if catch.scope not in entries_by_scope:
|
||||
failures.append(
|
||||
f"{rel_path}:{catch.line}: undocumented broad catch in {catch.scope}"
|
||||
)
|
||||
|
||||
for scope, entry in entries_by_scope.items():
|
||||
expected_count = entry.get("expected_count", 1)
|
||||
if not isinstance(expected_count, int) or expected_count < 1:
|
||||
failures.append(f"{rel_path}:{scope}: expected_count must be >= 1")
|
||||
continue
|
||||
actual_count = counts.get(scope, 0)
|
||||
if actual_count != expected_count:
|
||||
failures.append(
|
||||
f"{rel_path}:{scope}: expected {expected_count} broad catch(es), found {actual_count}"
|
||||
)
|
||||
|
||||
return failures
|
||||
|
||||
|
||||
def main() -> int:
|
||||
parser = argparse.ArgumentParser()
|
||||
parser.add_argument("--repo-root", default=".")
|
||||
parser.add_argument(
|
||||
"--policy",
|
||||
default="tests/exception_boundary_policy.json",
|
||||
)
|
||||
args = parser.parse_args()
|
||||
|
||||
repo_root = Path(args.repo_root).resolve()
|
||||
policy_path = repo_root / args.policy
|
||||
failures = validate_exception_boundary_policy(repo_root, load_policy(policy_path))
|
||||
if failures:
|
||||
for failure in failures:
|
||||
print(f"EXCEPTION-BOUNDARY-FAIL: {failure}")
|
||||
return 1
|
||||
print("EXCEPTION-BOUNDARY-PASS")
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main())
|
||||
@@ -274,3 +274,6 @@ def register_routes_once() -> None:
|
||||
_start_registration_retry_loop()
|
||||
except Exception:
|
||||
logging.getLogger("ComfyUI-OpenClaw").exception("Route registration failed")
|
||||
# CRITICAL: initial registration failures must fail closed. The retry loop is
|
||||
# only for PromptServer warm-up, not for hiding broken route/bootstrap state.
|
||||
raise
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
{
|
||||
"version": 1,
|
||||
"selected_modules": {
|
||||
"api/routes.py": {
|
||||
"broad_catches": [
|
||||
{
|
||||
"scope": "health_handler",
|
||||
"expected_count": 5,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Health diagnostics must degrade to partial snapshots when optional provider, metrics, event-store, or profile probes fail."
|
||||
},
|
||||
{
|
||||
"scope": "logs_tail_handler",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Log tail reads are operator diagnostics and return a bounded 500 response instead of crashing route handling."
|
||||
},
|
||||
{
|
||||
"scope": "register_dual_route._deprecated_handler",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Legacy-route telemetry must not break compatibility routing when metrics are unavailable."
|
||||
},
|
||||
{
|
||||
"scope": "register_dual_route",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Direct fallback route registration logs unexpected router-add failures without blocking PromptServer registration."
|
||||
},
|
||||
{
|
||||
"scope": "_resolve_mae_profile",
|
||||
"expected_count": 1,
|
||||
"classification": "needs_follow_up_test_coverage",
|
||||
"reason": "Runtime profile probing is best-effort today; future work should pin profile import/runtime failures more explicitly."
|
||||
},
|
||||
{
|
||||
"scope": "_run_mae_startup_gate",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "MAE posture validation is a startup diagnostic gate and logs unavailability without preventing core local loading."
|
||||
}
|
||||
]
|
||||
},
|
||||
"connector/router.py": {
|
||||
"broad_catches": [
|
||||
{
|
||||
"scope": "<module>",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Connector unit tests may stub the import graph; sanitizer import fallback keeps tests and standalone connector loading deterministic."
|
||||
},
|
||||
{
|
||||
"scope": "CommandRouter.handle",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Connector command dispatch is an external-message boundary and converts handler failures into a non-sensitive operator response."
|
||||
},
|
||||
{
|
||||
"scope": "CommandRouter._get_template_meta",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Template metadata lookup is best-effort; command execution falls back to explicit prompt input names when metadata is unavailable."
|
||||
}
|
||||
]
|
||||
},
|
||||
"services/route_bootstrap.py": {
|
||||
"broad_catches": [
|
||||
{
|
||||
"scope": "_register_plugins_and_shutdown_hooks",
|
||||
"expected_count": 1,
|
||||
"classification": "needs_follow_up_test_coverage",
|
||||
"reason": "Plugin/shutdown hook registration is currently non-fatal but should gain narrower plugin failure coverage."
|
||||
},
|
||||
{
|
||||
"scope": "_initialize_registries_and_security_gate",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Bootstrap initialization logs context and re-raises so security gate failures remain fail-closed."
|
||||
},
|
||||
{
|
||||
"scope": "_start_registration_retry_loop._retry_worker",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Background route retry is a PromptServer warm-up boundary and keeps retrying after transient registration failures."
|
||||
},
|
||||
{
|
||||
"scope": "register_routes_once",
|
||||
"expected_count": 1,
|
||||
"classification": "allowed_boundary_guard",
|
||||
"reason": "Initial route registration logs context and re-raises so unexpected bootstrap failures are visible and fail-closed."
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,64 @@
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from services import route_bootstrap
|
||||
|
||||
|
||||
class TestExceptionBoundaryGovernance(unittest.TestCase):
|
||||
def test_register_routes_once_reraises_initial_registration_failure(self):
|
||||
route_bootstrap._routes_registered = False
|
||||
server = SimpleNamespace(app=object())
|
||||
prompt_server = SimpleNamespace(instance=server)
|
||||
|
||||
with (
|
||||
patch("services.route_bootstrap._register_plugins_and_shutdown_hooks"),
|
||||
patch("services.route_bootstrap._initialize_registries_and_security_gate"),
|
||||
patch(
|
||||
"services.route_bootstrap._do_full_registration",
|
||||
side_effect=RuntimeError("route-registration-broken"),
|
||||
),
|
||||
patch("services.route_bootstrap.logging.getLogger", return_value=MagicMock()),
|
||||
patch.dict(
|
||||
sys.modules,
|
||||
{"server": SimpleNamespace(PromptServer=prompt_server)},
|
||||
),
|
||||
):
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
route_bootstrap.register_routes_once()
|
||||
|
||||
self.assertIn("route-registration-broken", str(ctx.exception))
|
||||
self.assertFalse(route_bootstrap._routes_registered)
|
||||
|
||||
def test_selected_module_broad_catches_match_exception_policy(self):
|
||||
from scripts.verify_exception_boundary_policy import (
|
||||
load_policy,
|
||||
validate_exception_boundary_policy,
|
||||
)
|
||||
|
||||
repo_root = Path(__file__).resolve().parents[1]
|
||||
failures = validate_exception_boundary_policy(
|
||||
repo_root,
|
||||
load_policy(repo_root / "tests" / "exception_boundary_policy.json"),
|
||||
)
|
||||
|
||||
self.assertEqual(failures, [])
|
||||
|
||||
def test_connector_trust_parsing_no_longer_uses_broad_exception(self):
|
||||
from scripts.verify_exception_boundary_policy import iter_broad_catches
|
||||
|
||||
repo_root = Path(__file__).resolve().parents[1]
|
||||
catches = list(iter_broad_catches(repo_root / "connector" / "router.py"))
|
||||
broad_scopes = {
|
||||
catch.scope
|
||||
for catch in catches
|
||||
if catch.scope == "CommandRouter._is_trusted"
|
||||
}
|
||||
|
||||
self.assertEqual(broad_scopes, set())
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user