mirror of
https://github.com/rookiestar28/ComfyUI-OpenClaw.git
synced 2026-08-14 00:48:07 +00:00
fix(robustness): harden exception boundaries
This commit is contained in:
+9
-18
@@ -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"},
|
||||
|
||||
+4
-1
@@ -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"
|
||||
|
||||
|
||||
@@ -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 {}
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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}"
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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": "<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."
|
||||
"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"
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user