diff --git a/connector/router.py b/connector/router.py index e5ac9a7..258460e 100644 --- a/connector/router.py +++ b/connector/router.py @@ -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 diff --git a/scripts/verify_exception_boundary_policy.py b/scripts/verify_exception_boundary_policy.py new file mode 100644 index 0000000..dd19be3 --- /dev/null +++ b/scripts/verify_exception_boundary_policy.py @@ -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 "", + 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()) diff --git a/services/route_bootstrap.py b/services/route_bootstrap.py index 88c08fa..66d0125 100644 --- a/services/route_bootstrap.py +++ b/services/route_bootstrap.py @@ -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 diff --git a/tests/exception_boundary_policy.json b/tests/exception_boundary_policy.json new file mode 100644 index 0000000..8d8aa5d --- /dev/null +++ b/tests/exception_boundary_policy.json @@ -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": "", + "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." + } + ] + } + } +} diff --git a/tests/test_r180_exception_boundary_governance.py b/tests/test_r180_exception_boundary_governance.py new file mode 100644 index 0000000..7e5c76d --- /dev/null +++ b/tests/test_r180_exception_boundary_governance.py @@ -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()