From b68d951fd057b187118d01fb30507b8f976db3be Mon Sep 17 00:00:00 2001 From: rookiestar28 Date: Sat, 11 Jul 2026 07:25:15 +0800 Subject: [PATCH] fix(robustness): harden exception boundaries --- api/config.py | 27 +- api/routes.py | 5 +- connector/platforms/feishu_webhook.py | 2 +- connector/platforms/slack_webhook.py | 6 +- scripts/verify_exception_boundary_policy.py | 106 +++++- services/route_bootstrap.py | 43 ++- tests/exception_boundary_policy.json | 189 ++++++++-- tests/static_analysis_policy.json | 9 +- tests/test_r219_exception_boundary_phase2.py | 358 +++++++++++++++++++ 9 files changed, 682 insertions(+), 63 deletions(-) create mode 100644 tests/test_r219_exception_boundary_phase2.py diff --git a/api/config.py b/api/config.py index 65584f2..af5081c 100644 --- a/api/config.py +++ b/api/config.py @@ -331,11 +331,11 @@ async def config_get_handler(request: web.Request) -> web.Response: status=403, ) except Exception as e: - logger.exception("Error getting config") + logger.error("Error getting config (error_type=%s)", type(e).__name__) return web.json_response( { "ok": False, - "error": str(e), + "error": "config_read_failed", }, status=500, ) @@ -865,7 +865,7 @@ async def llm_test_handler(request: web.Request) -> web.Response: ): try: timeout_sec = int(timeout_val) - except Exception: + except (TypeError, ValueError, OverflowError): return web.json_response( {"ok": False, "error": "timeout_sec must be an integer"}, status=400, @@ -879,7 +879,7 @@ async def llm_test_handler(request: web.Request) -> web.Response: ): try: max_retries = int(retries_val) - except Exception: + except (TypeError, ValueError, OverflowError): return web.json_response( {"ok": False, "error": "max_retries must be an integer"}, status=400, @@ -963,20 +963,20 @@ async def llm_test_handler(request: web.Request) -> web.Response: status=403, ) except Exception as e: - logger.exception("LLM test failed") + logger.error("LLM test failed (error_type=%s)", type(e).__name__) emit_audit_event( action="llm.test_connection", target="llm", outcome="error", token_info=token_info, status_code=500, - details={"error": str(e)}, + details={"error": "llm_test_failed"}, request=request, ) return web.json_response( { "ok": False, - "error": str(e), + "error": "llm_test_failed", }, status=500, ) @@ -1127,19 +1127,10 @@ async def llm_chat_handler(request: web.Request) -> web.Response: payload["retry_after"] = e.retry_after return web.json_response(payload, status=e.status_code) except Exception as e: - # S29: Redact exception message to prevent accidental prompt content leakage. - # Downgraded from error → warning (non-actionable for operators when provider-specific). - try: - from services.redaction import redact_text # type: ignore - except ImportError: - try: - from ..services.redaction import redact_text - except ImportError: - redact_text = str # type: ignore + # S29: classify only; preserve the explicit redaction marker for log consumers. logger.warning( - "LLM chat request failed: %s: %s", + "LLM chat request failed: ***REDACTED*** (error_type=%s)", type(e).__name__, - redact_text(str(e)), ) return web.json_response( {"ok": False, "error": "llm_request_failed"}, diff --git a/api/routes.py b/api/routes.py index 7642ab2..962bd73 100644 --- a/api/routes.py +++ b/api/routes.py @@ -972,7 +972,10 @@ def _resolve_mae_profile() -> str: runtime_profile = get_runtime_profile().value if runtime_profile == "hardened": return "hardened" - except Exception: + except ImportError: + # IMPORTANT: optional standalone import absence may fall back to the deployment + # profile, but unexpected resolver failures must propagate instead of downgrading + # hardened posture silently. pass return profile or "local" diff --git a/connector/platforms/feishu_webhook.py b/connector/platforms/feishu_webhook.py index 460c461..ca1607a 100644 --- a/connector/platforms/feishu_webhook.py +++ b/connector/platforms/feishu_webhook.py @@ -182,7 +182,7 @@ def _json_loads_safe(raw: str) -> Dict[str, Any]: parsed = json.loads(raw) if isinstance(parsed, dict): return parsed - except Exception: + except (TypeError, ValueError): pass return {} diff --git a/connector/platforms/slack_webhook.py b/connector/platforms/slack_webhook.py index 6432007..1b233f2 100644 --- a/connector/platforms/slack_webhook.py +++ b/connector/platforms/slack_webhook.py @@ -128,7 +128,7 @@ def _json_loads_safe(raw: Any) -> Dict[str, Any]: try: parsed = json.loads(raw) return parsed if isinstance(parsed, dict) else {} - except Exception: + except (TypeError, ValueError): return {} @@ -685,7 +685,9 @@ class SlackWebhookServer: }, ) except Exception as e: - logger.exception(f"Error handling Slack event: {e}") + logger.error( + "Slack event handling failed (error_type=%s)", type(e).__name__ + ) async def process_interaction_payload(self, payload: Dict[str, Any]) -> bool: interaction_type = str(payload.get("type", "") or "").strip() diff --git a/scripts/verify_exception_boundary_policy.py b/scripts/verify_exception_boundary_policy.py index dd19be3..232d9a0 100644 --- a/scripts/verify_exception_boundary_policy.py +++ b/scripts/verify_exception_boundary_policy.py @@ -12,6 +12,7 @@ import ast import json from collections import Counter from dataclasses import dataclass +from datetime import date from pathlib import Path from typing import Any, Iterable @@ -20,6 +21,7 @@ VALID_CLASSIFICATIONS = { "needs_narrowing", "needs_follow_up_test_coverage", } +VALID_COVERAGE_MODES = {"all_broad_catches", "selected_scopes"} @dataclass(frozen=True) @@ -93,11 +95,26 @@ def validate_exception_boundary_policy( policy: dict[str, Any], ) -> list[str]: failures: list[str] = [] + if set(policy) != {"version", "selected_modules"}: + failures.append("policy root keys must match the version 2 schema") + if policy.get("version") != 2: + failures.append("policy version must equal 2") 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()): + rel_path_obj = Path(rel_path) + if ( + rel_path_obj.is_absolute() + or ".." in rel_path_obj.parts + or rel_path_obj.suffix != ".py" + ): + failures.append(f"{rel_path}: unsafe selected module path") + continue + if not isinstance(module_policy, dict): + failures.append(f"{rel_path}: module policy must be an object") + continue path = repo_root / rel_path if not path.is_file(): failures.append(f"{rel_path}: selected module does not exist") @@ -108,14 +125,60 @@ def validate_exception_boundary_policy( failures.append(f"{rel_path}: broad_catches must be a list") continue + coverage = module_policy.get("coverage") + if coverage not in VALID_COVERAGE_MODES: + failures.append(f"{rel_path}: invalid coverage mode {coverage!r}") + continue + expected_module_keys = {"coverage", "broad_catches"} + if coverage == "selected_scopes": + expected_module_keys.add("selected_scopes") + if set(module_policy) != expected_module_keys: + failures.append(f"{rel_path}: module keys must match coverage schema") + selected_scopes_raw = module_policy.get("selected_scopes", []) + if coverage == "selected_scopes": + if ( + not isinstance(selected_scopes_raw, list) + or not selected_scopes_raw + or any( + not isinstance(scope, str) or not scope + for scope in selected_scopes_raw + ) + or len(selected_scopes_raw) != len(set(selected_scopes_raw)) + ): + failures.append( + f"{rel_path}: selected_scopes must be a unique non-empty string list" + ) + selected_scopes: set[str] = set() + else: + selected_scopes = set(selected_scopes_raw) + else: + if selected_scopes_raw: + failures.append( + f"{rel_path}: all_broad_catches must not declare selected_scopes" + ) + selected_scopes = set() + 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 + if set(entry) != { + "scope", + "expected_count", + "classification", + "reason", + "regression_owner", + "review_after", + }: + failures.append( + f"{rel_path}: broad_catches[{index}] entry keys must match schema" + ) scope = entry.get("scope") classification = entry.get("classification") reason = entry.get("reason") + regression_owner = entry.get("regression_owner") + review_after = entry.get("review_after") if not isinstance(scope, str) or not scope: failures.append(f"{rel_path}: broad_catches[{index}] missing scope") continue @@ -128,10 +191,49 @@ def validate_exception_boundary_policy( ) if not isinstance(reason, str) or not reason.strip(): failures.append(f"{rel_path}:{scope}: missing reason") + if not isinstance(regression_owner, str) or not regression_owner.strip(): + failures.append(f"{rel_path}:{scope}: missing regression_owner") + else: + owner_path = Path(regression_owner) + if ( + owner_path.is_absolute() + or ".." in owner_path.parts + or not owner_path.parts + or owner_path.parts[0] != "tests" + or owner_path.suffix != ".py" + ): + failures.append( + f"{rel_path}:{scope}: regression_owner must be a safe tests/*.py path" + ) + elif not (repo_root / owner_path).is_file(): + failures.append( + f"{rel_path}:{scope}: regression_owner does not exist" + ) + try: + if not isinstance(review_after, str): + raise TypeError + review_date = date.fromisoformat(review_after) + except (TypeError, ValueError): + failures.append(f"{rel_path}:{scope}: invalid review_after") + else: + if review_date < date.today(): + failures.append(f"{rel_path}:{scope}: review_after is expired") + if coverage == "selected_scopes" and scope not in selected_scopes: + failures.append(f"{rel_path}:{scope}: entry is outside selected_scopes") catches = tuple(iter_broad_catches(path)) - counts = Counter(catch.scope for catch in catches) - for catch in catches: + governed_catches = ( + catches + if coverage == "all_broad_catches" + else tuple(catch for catch in catches if catch.scope in selected_scopes) + ) + counts = Counter(catch.scope for catch in governed_catches) + if coverage == "selected_scopes": + for stale_scope in sorted(selected_scopes - set(counts)): + failures.append( + f"{rel_path}:{stale_scope}: selected scope has no broad catch" + ) + for catch in governed_catches: if catch.scope not in entries_by_scope: failures.append( f"{rel_path}:{catch.line}: undocumented broad catch in {catch.scope}" diff --git a/services/route_bootstrap.py b/services/route_bootstrap.py index ad6472e..00266da 100644 --- a/services/route_bootstrap.py +++ b/services/route_bootstrap.py @@ -65,16 +65,41 @@ def _mark_startup_fatal(phase: str, exc: BaseException) -> None: ) -def _register_plugins_and_shutdown_hooks() -> None: - # R67: Best-effort process shutdown hook for scheduler/failover flush. - try: - from .plugins.builtin import register_all - from .runtime_lifecycle import register_shutdown_hooks +def _load_plugin_shutdown_registrars(): + """Load optional startup registrars behind one patchable compatibility seam.""" - register_shutdown_hooks() - register_all() - except Exception as e: - logging.getLogger("ComfyUI-OpenClaw").error(f"Failed to register plugins: {e}") + from .plugins.builtin import register_all + from .runtime_lifecycle import register_shutdown_hooks + + return register_shutdown_hooks, register_all + + +def _register_plugins_and_shutdown_hooks() -> None: + # R67: Best-effort process shutdown hook and built-in plugin registration. + try: + register_shutdown_hooks, register_all = _load_plugin_shutdown_registrars() + except ImportError as exc: + logging.getLogger("ComfyUI-OpenClaw").error( + "Optional startup registrars unavailable (error_type=%s)", + type(exc).__name__, + ) + return + + logger = logging.getLogger("ComfyUI-OpenClaw") + for component, registrar in ( + ("shutdown_hooks", register_shutdown_hooks), + ("builtin_plugins", register_all), + ): + try: + registrar() + except Exception as exc: + # IMPORTANT: these optional steps are independent. Keep startup available, + # do not echo exception content, and do not catch BaseException cancellation. + logger.error( + "Optional startup registrar failed (component=%s, error_type=%s)", + component, + type(exc).__name__, + ) def _initialize_registries_and_security_gate() -> None: diff --git a/tests/exception_boundary_policy.json b/tests/exception_boundary_policy.json index 248bfea..6438ab2 100644 --- a/tests/exception_boundary_policy.json +++ b/tests/exception_boundary_policy.json @@ -1,105 +1,250 @@ { - "version": 1, + "version": 2, "selected_modules": { "api/routes.py": { + "coverage": "all_broad_catches", "broad_catches": [ { "scope": "health_handler", "expected_count": 6, "classification": "allowed_boundary_guard", - "reason": "Health diagnostics must degrade to partial snapshots when optional provider, metrics, startup-lifecycle, event-store, or profile probes fail." + "reason": "Health diagnostics degrade to partial snapshots when optional probes fail.", + "regression_owner": "tests/contract/test_api_contract.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Log-tail diagnostics return a bounded failure instead of crashing route handling.", + "regression_owner": "tests/test_r117_observability_redaction_endpoints.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Legacy-route telemetry must not break compatibility routing.", + "regression_owner": "tests/test_r180_exception_boundary_governance.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Fallback registration retains compatibility when optional router integration fails.", + "regression_owner": "tests/test_r180_exception_boundary_governance.py", + "review_after": "2027-01-11" }, { "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." + "reason": "MAE import availability is an optional startup diagnostic boundary.", + "regression_owner": "tests/test_r219_exception_boundary_phase2.py", + "review_after": "2027-01-11" } ] }, "connector/router.py": { + "coverage": "all_broad_catches", "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." + "reason": "Standalone connector tests may stub the sanitizer import graph.", + "regression_owner": "tests/connector/test_router_hotspot_r181.py", + "review_after": "2027-01-11" }, { "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." + "reason": "External command dispatch translates handler failures to a fixed operator response.", + "regression_owner": "tests/connector/test_router_hotspot_r181.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Template metadata lookup is best-effort and has an explicit prompt-name fallback.", + "regression_owner": "tests/connector/test_router_hotspot_r181.py", + "review_after": "2027-01-11" } ] }, "services/route_bootstrap.py": { + "coverage": "all_broad_catches", "broad_catches": [ { "scope": "_mark_startup_ready_and_start_warmups", "expected_count": 1, "classification": "allowed_boundary_guard", - "reason": "Optional warmup diagnostics must not block route availability after required startup succeeds." + "reason": "Optional warmup diagnostics must not undo successful route startup.", + "regression_owner": "tests/test_r188_startup_lifecycle.py", + "review_after": "2027-01-11" }, { "scope": "_mark_startup_fatal", "expected_count": 1, "classification": "allowed_boundary_guard", - "reason": "Fatal startup-state recording is diagnostic only and must not mask the original bootstrap exception." + "reason": "Fatal-state diagnostics must not mask the original bootstrap exception.", + "regression_owner": "tests/test_r188_startup_lifecycle.py", + "review_after": "2027-01-11" }, { "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." + "classification": "allowed_boundary_guard", + "reason": "Independent optional registrars log content-free failure classifications while startup continues.", + "regression_owner": "tests/test_r219_exception_boundary_phase2.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Bootstrap initialization logs context and re-raises fail-closed.", + "regression_owner": "tests/test_r180_exception_boundary_governance.py", + "review_after": "2027-01-11" }, { "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." + "reason": "Background route warmup retries after transient registration failures.", + "regression_owner": "tests/test_route_registration.py", + "review_after": "2027-01-11" }, { "scope": "register_routes_once", "expected_count": 2, "classification": "allowed_boundary_guard", - "reason": "Required startup and initial route registration log context and re-raise so unexpected bootstrap failures are visible and fail-closed." + "reason": "Required startup failures are recorded and re-raised instead of hidden.", + "regression_owner": "tests/test_r180_exception_boundary_governance.py", + "review_after": "2027-01-11" + } + ] + }, + "api/config.py": { + "coverage": "selected_scopes", + "selected_scopes": [ + "config_get_handler", + "llm_test_handler", + "llm_chat_handler" + ], + "broad_catches": [ + { + "scope": "config_get_handler", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "The admin read boundary returns a fixed content-free 500 code after tenant-specific failures.", + "regression_owner": "tests/test_r219_exception_boundary_phase2.py", + "review_after": "2027-01-11" + }, + { + "scope": "llm_test_handler", + "expected_count": 2, + "classification": "allowed_boundary_guard", + "reason": "Request decoding and final LLM test translation retain existing response shape with fixed errors.", + "regression_owner": "tests/test_r219_exception_boundary_phase2.py", + "review_after": "2027-01-11" + }, + { + "scope": "llm_chat_handler", + "expected_count": 2, + "classification": "allowed_boundary_guard", + "reason": "Chat request decoding and final server-error translation are public route boundaries.", + "regression_owner": "tests/test_r219_exception_boundary_phase2.py", + "review_after": "2027-01-11" + } + ] + }, + "connector/platforms/slack_webhook.py": { + "coverage": "selected_scopes", + "selected_scopes": [ + "SlackWebhookServer.handle_oauth_callback", + "SlackWebhookServer.handle_event", + "SlackWebhookServer.handle_interaction", + "SlackWebhookServer.process_event_payload", + "SlackWebhookServer.process_interaction_payload" + ], + "broad_catches": [ + { + "scope": "SlackWebhookServer.handle_oauth_callback", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "OAuth callback failures translate to a constant external response.", + "regression_owner": "tests/test_f58_slack_oauth_installations.py", + "review_after": "2027-01-11" + }, + { + "scope": "SlackWebhookServer.handle_event", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Request-read failures translate to the existing fixed 400 response.", + "regression_owner": "tests/test_f57_slack_transport_parity.py", + "review_after": "2027-01-11" + }, + { + "scope": "SlackWebhookServer.handle_interaction", + "expected_count": 2, + "classification": "allowed_boundary_guard", + "reason": "Request-read and routed interaction failures retain fixed acknowledgement responses.", + "regression_owner": "tests/test_f59_slack_interactions.py", + "review_after": "2027-01-11" + }, + { + "scope": "SlackWebhookServer.process_event_payload", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Slack event dispatch is acknowledged while failures log only a safe type classification.", + "regression_owner": "tests/test_f57_slack_transport_parity.py", + "review_after": "2027-01-11" + }, + { + "scope": "SlackWebhookServer.process_interaction_payload", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Pre-commit router failures release replay claims and re-raise for fixed outer translation.", + "regression_owner": "tests/test_f59_slack_interactions.py", + "review_after": "2027-01-11" + } + ] + }, + "connector/platforms/feishu_webhook.py": { + "coverage": "selected_scopes", + "selected_scopes": [ + "FeishuWebhookServer.handle_event", + "FeishuWebhookServer.handle_callback", + "FeishuWebhookServer.process_callback_payload" + ], + "broad_catches": [ + { + "scope": "FeishuWebhookServer.handle_event", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Request-read failures translate to the existing fixed 400 response.", + "regression_owner": "tests/test_f68_feishu_installations.py", + "review_after": "2027-01-11" + }, + { + "scope": "FeishuWebhookServer.handle_callback", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Callback request-read failures translate to the existing fixed 400 response.", + "regression_owner": "tests/test_f69_feishu_callbacks.py", + "review_after": "2027-01-11" + }, + { + "scope": "FeishuWebhookServer.process_callback_payload", + "expected_count": 1, + "classification": "allowed_boundary_guard", + "reason": "Pre-completion router failures release the callback request for retry and re-raise.", + "regression_owner": "tests/test_f69_feishu_callbacks.py", + "review_after": "2027-01-11" } ] } diff --git a/tests/static_analysis_policy.json b/tests/static_analysis_policy.json index 29845ad..b02c40d 100644 --- a/tests/static_analysis_policy.json +++ b/tests/static_analysis_policy.json @@ -119,19 +119,12 @@ "message": "Unused \"type: ignore\" comment", "count": 6 }, - { - "tool": "mypy", - "path": "api/config.py", - "code": "no-redef", - "message": "Name \"redact_text\" already defined (possibly by an import)", - "count": 1 - }, { "tool": "mypy", "path": "api/config.py", "code": "unused-ignore", "message": "Unused \"type: ignore\" comment", - "count": 7 + "count": 6 }, { "tool": "mypy", diff --git a/tests/test_r219_exception_boundary_phase2.py b/tests/test_r219_exception_boundary_phase2.py new file mode 100644 index 0000000..3db014c --- /dev/null +++ b/tests/test_r219_exception_boundary_phase2.py @@ -0,0 +1,358 @@ +"""R219 exception-boundary phase-2 regression contracts.""" + +from __future__ import annotations + +import copy +import json +import os +import tempfile +import unittest +from contextlib import nullcontext +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +from api import config as config_api +from api import routes +from connector.platforms import feishu_webhook, slack_webhook +from services import route_bootstrap +from services.runtime_profile import RuntimeProfile + +ROOT = Path(__file__).resolve().parents[1] +POLICY_PATH = ROOT / "tests" / "exception_boundary_policy.json" + + +class TestPolicyV2(unittest.TestCase): + def test_repository_policy_v2_is_complete_and_has_no_open_followups(self): + from scripts.verify_exception_boundary_policy import ( + load_policy, + validate_exception_boundary_policy, + ) + + policy = load_policy(POLICY_PATH) + self.assertEqual(policy["version"], 2) + self.assertEqual( + set(policy["selected_modules"]), + { + "api/routes.py", + "connector/router.py", + "services/route_bootstrap.py", + "api/config.py", + "connector/platforms/slack_webhook.py", + "connector/platforms/feishu_webhook.py", + }, + ) + for module in policy["selected_modules"].values(): + self.assertIn(module["coverage"], {"all_broad_catches", "selected_scopes"}) + for entry in module["broad_catches"]: + self.assertTrue(entry["regression_owner"]) + self.assertRegex(entry["review_after"], r"^\d{4}-\d{2}-\d{2}$") + self.assertNotEqual( + entry["classification"], "needs_follow_up_test_coverage" + ) + self.assertEqual(validate_exception_boundary_policy(ROOT, policy), []) + + def test_policy_rejects_expiry_missing_owner_and_new_selected_scope_catch(self): + from scripts.verify_exception_boundary_policy import ( + validate_exception_boundary_policy, + ) + + policy = json.loads(POLICY_PATH.read_text(encoding="utf-8")) + expired = copy.deepcopy(policy) + first = expired["selected_modules"]["api/routes.py"]["broad_catches"][0] + first["review_after"] = "2020-01-01" + self.assertTrue( + any( + "expired" in item + for item in validate_exception_boundary_policy(ROOT, expired) + ) + ) + + missing_owner = copy.deepcopy(policy) + del missing_owner["selected_modules"]["api/routes.py"]["broad_catches"][0][ + "regression_owner" + ] + self.assertTrue( + any( + "regression_owner" in item + for item in validate_exception_boundary_policy(ROOT, missing_owner) + ) + ) + + missing_owner_file = copy.deepcopy(policy) + missing_owner_file["selected_modules"]["api/routes.py"]["broad_catches"][0][ + "regression_owner" + ] = "tests/does_not_exist.py" + self.assertTrue( + any( + "does not exist" in item + for item in validate_exception_boundary_policy(ROOT, missing_owner_file) + ) + ) + + stale_scope = copy.deepcopy(policy) + stale_scope["selected_modules"]["api/config.py"]["selected_scopes"].append( + "removed_scope" + ) + self.assertTrue( + any( + "selected scope has no broad catch" in item + for item in validate_exception_boundary_policy(ROOT, stale_scope) + ) + ) + + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) + (repo / "selected.py").write_text( + "def governed():\n try:\n return 1\n except Exception:\n return 0\n", + encoding="utf-8", + ) + selected_policy = { + "version": 2, + "selected_modules": { + "selected.py": { + "coverage": "selected_scopes", + "broad_catches": [], + "selected_scopes": ["governed"], + } + }, + } + failures = validate_exception_boundary_policy(repo, selected_policy) + self.assertTrue(any("undocumented broad catch" in item for item in failures)) + + def test_policy_rejects_unknown_schema_keys_and_unsafe_module_paths(self): + from scripts.verify_exception_boundary_policy import ( + validate_exception_boundary_policy, + ) + + policy = json.loads(POLICY_PATH.read_text(encoding="utf-8")) + unknown_entry = copy.deepcopy(policy) + unknown_entry["selected_modules"]["api/routes.py"]["broad_catches"][0][ + "unexpected" + ] = True + self.assertTrue( + any( + "entry keys" in item + for item in validate_exception_boundary_policy(ROOT, unknown_entry) + ) + ) + + unsafe_path = copy.deepcopy(policy) + unsafe_path["selected_modules"]["../outside.py"] = unsafe_path[ + "selected_modules" + ].pop("api/routes.py") + self.assertTrue( + any( + "unsafe selected module path" in item + for item in validate_exception_boundary_policy(ROOT, unsafe_path) + ) + ) + + +class TestMaeProfileBoundary(unittest.TestCase): + def test_explicit_deployment_profile_precedes_runtime_probe(self): + with ( + patch.dict( + os.environ, {"OPENCLAW_DEPLOYMENT_PROFILE": "public"}, clear=True + ), + patch( + "services.runtime_profile.get_runtime_profile", + side_effect=RuntimeError("must-not-run"), + ) as probe, + ): + self.assertEqual(routes._resolve_mae_profile(), "public") + probe.assert_not_called() + + def test_missing_profile_import_falls_back_but_unexpected_failure_propagates(self): + with patch.dict(os.environ, {}, clear=True): + with patch( + "services.runtime_profile.get_runtime_profile", + return_value=RuntimeProfile.HARDENED, + ): + self.assertEqual(routes._resolve_mae_profile(), "hardened") + + with patch( + "services.runtime_profile.get_runtime_profile", + side_effect=ImportError("optional-profile-import"), + ): + self.assertEqual(routes._resolve_mae_profile(), "local") + + with patch( + "services.runtime_profile.get_runtime_profile", + side_effect=RuntimeError("profile-resolution-failed"), + ): + with self.assertRaisesRegex(RuntimeError, "profile-resolution-failed"): + routes._resolve_mae_profile() + + +class TestOptionalStartupRegistrars(unittest.TestCase): + def test_shutdown_and_plugins_are_independent_and_logs_are_content_free(self): + shutdown = MagicMock(side_effect=RuntimeError("sensitive-shutdown-detail")) + plugins = MagicMock(side_effect=ValueError("sensitive-plugin-detail")) + logger = MagicMock() + with ( + patch.object( + route_bootstrap, + "_load_plugin_shutdown_registrars", + return_value=(shutdown, plugins), + ), + patch("services.route_bootstrap.logging.getLogger", return_value=logger), + ): + route_bootstrap._register_plugins_and_shutdown_hooks() + + shutdown.assert_called_once_with() + plugins.assert_called_once_with() + rendered_calls = repr(logger.method_calls) + self.assertIn("RuntimeError", rendered_calls) + self.assertIn("ValueError", rendered_calls) + self.assertNotIn("sensitive-shutdown-detail", rendered_calls) + self.assertNotIn("sensitive-plugin-detail", rendered_calls) + + def test_baseexception_from_optional_registrar_propagates(self): + shutdown = MagicMock(side_effect=KeyboardInterrupt("cancel")) + plugins = MagicMock() + with patch.object( + route_bootstrap, + "_load_plugin_shutdown_registrars", + return_value=(shutdown, plugins), + ): + with self.assertRaises(KeyboardInterrupt): + route_bootstrap._register_plugins_and_shutdown_hooks() + plugins.assert_not_called() + + def test_registrar_import_failure_is_content_free(self): + logger = MagicMock() + with ( + patch.object( + route_bootstrap, + "_load_plugin_shutdown_registrars", + side_effect=ImportError("sensitive-import-detail"), + ), + patch("services.route_bootstrap.logging.getLogger", return_value=logger), + ): + route_bootstrap._register_plugins_and_shutdown_hooks() + rendered_calls = repr(logger.method_calls) + self.assertIn("ImportError", rendered_calls) + self.assertNotIn("sensitive-import-detail", rendered_calls) + + +class TestExpectedParserBoundaries(unittest.TestCase): + def test_slack_and_feishu_json_helpers_narrow_expected_parse_errors(self): + self.assertEqual(slack_webhook._json_loads_safe("{"), {}) + self.assertEqual(feishu_webhook._json_loads_safe("{"), {}) + + for module, helper in ( + (slack_webhook, slack_webhook._json_loads_safe), + (feishu_webhook, feishu_webhook._json_loads_safe), + ): + with self.subTest(module=module.__name__): + with patch.object( + module.json, "loads", side_effect=RuntimeError("parser-defect") + ): + with self.assertRaisesRegex(RuntimeError, "parser-defect"): + helper("{}") + with patch.object( + module.json, "loads", side_effect=KeyboardInterrupt("cancel") + ): + with self.assertRaises(KeyboardInterrupt): + helper("{}") + + def test_config_numeric_parsers_have_no_broad_catch(self): + from scripts.verify_exception_boundary_policy import iter_broad_catches + + catches = list(iter_broad_catches(ROOT / "api" / "config.py")) + llm_test = [catch for catch in catches if catch.scope == "llm_test_handler"] + self.assertEqual(len(llm_test), 2) + + +class TestFixedConfigErrorTranslation(unittest.IsolatedAsyncioTestCase): + async def test_config_read_failure_uses_fixed_code_without_echo(self): + request = MagicMock() + web = MagicMock() + web.json_response.side_effect = lambda body, status=200: (body, status) + logger = MagicMock() + tenant = SimpleNamespace(tenant_id="default") + with ( + patch.object(config_api, "web", web), + patch.object( + config_api, "require_observability_access", return_value=(True, None) + ), + patch.object(config_api, "check_rate_limit", return_value=True), + patch.object( + config_api, "resolve_token_info", return_value=SimpleNamespace() + ), + patch.object( + config_api, "request_tenant_scope", return_value=nullcontext(tenant) + ), + patch.object( + config_api, + "get_effective_config", + side_effect=RuntimeError("sensitive-config-detail"), + ), + patch.object(config_api, "logger", logger), + ): + body, status = await config_api.config_get_handler(request) + + self.assertEqual(status, 500) + self.assertEqual(body, {"ok": False, "error": "config_read_failed"}) + self.assertNotIn("sensitive-config-detail", repr(logger.method_calls)) + + async def test_llm_test_failure_uses_fixed_response_audit_and_log(self): + request = MagicMock() + request.json = AsyncMock(return_value={}) + web = MagicMock() + web.json_response.side_effect = lambda body, status=200: (body, status) + logger = MagicMock() + audit = MagicMock() + tenant = SimpleNamespace(tenant_id="default") + with ( + patch.object(config_api, "web", web), + patch.object(config_api, "get_admin_token", return_value="configured"), + patch.object( + config_api, "require_same_origin_if_no_token", return_value=None + ), + patch.object(config_api, "check_rate_limit", return_value=True), + patch.object( + config_api, "resolve_token_info", return_value=SimpleNamespace() + ), + patch.object(config_api, "require_admin_token", return_value=(True, None)), + patch.object( + config_api, "request_tenant_scope", return_value=nullcontext(tenant) + ), + patch.object( + config_api, + "LLMClient", + side_effect=RuntimeError("sensitive-llm-detail"), + ), + patch.object(config_api, "emit_audit_event", audit), + patch.object(config_api, "logger", logger), + ): + body, status = await config_api.llm_test_handler(request) + + self.assertEqual(status, 500) + self.assertEqual(body, {"ok": False, "error": "llm_test_failed"}) + combined = repr(logger.method_calls) + repr(audit.call_args_list) + self.assertIn("llm_test_failed", combined) + self.assertNotIn("sensitive-llm-detail", combined) + + +class TestSelectedConnectorLogging(unittest.IsolatedAsyncioTestCase): + async def test_slack_event_failure_logs_type_without_exception_content(self): + from tests.test_r124_slack_ingress_contract import ( + _make_event_payload, + _make_server, + ) + + server = _make_server(require_mention=False) + server.router.handle.side_effect = RuntimeError("sensitive-slack-detail") + logger = MagicMock() + with patch.object(slack_webhook, "logger", logger): + await server.process_event_payload(_make_event_payload()) + + rendered_calls = repr(logger.method_calls) + self.assertIn("RuntimeError", rendered_calls) + self.assertNotIn("sensitive-slack-detail", rendered_calls) + + +if __name__ == "__main__": + unittest.main()