From 6eff6561a9d51bbfc9e84242b1799d5ec33997a5 Mon Sep 17 00:00:00 2001 From: Amit Avital Date: Sat, 1 Aug 2026 19:59:34 +0300 Subject: [PATCH] fix(engine): stop a failed child-agent call from killing the whole run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An OrchestratorNode child-agent call had no exception handling at all, unlike its sibling AgentNode: an uncaught error (a validation failure on malformed tool args, a downed child model, ...) crashed the entire run with a bare 500 and no assistant reply. - OrchestratorNode.invoke_tool and _make_tool.invoke now catch child failures, log them, and return a message so the model can recover instead of the run dying. GraphInterrupt (HITL) and HookExecutionError from a fail-closed hook (e.g. before_tool_call) are re-raised — a security gate rejecting a call must still abort the run, not be reported as a recoverable child failure. - Hook failure_policy (fail-closed vs warn) is now a proper FailurePolicy enum instead of a magic string, with a per-hook-point default table. on_tool_error now defaults to warn: a broken error-reporting hook should not itself crash an otherwise-recoverable run, while security-critical points (before_tool_call, etc.) stay fail-closed. - Exception text logged anywhere (engine run/stream failures, the observability trace callback, tool-call failures) now goes through a new safe_error()/log() path instead of raw str(exc). A pydantic ValidationError's str() echoes back the rejected input_value — the caller's own arguments, which may hold credentials — so error logging reduces it to field names/reasons instead of the raw payload. - HookExecutionError's own message no longer repeats the wrapped cause's text, for the same reason. Co-Authored-By: Claude Sonnet 5 --- docs/RUNTIME_HOOKS.md | 17 ++-- docs/runtime-hooks.mdx | 4 +- examples/config.schema.json | 3 +- src/agent_engine/core/spec.py | 16 ++-- src/agent_engine/engine/langgraph/engine.py | 2 + src/agent_engine/engine/langgraph/nodes.py | 46 +++++++++- src/agent_engine/logging_config.py | 28 +++++- .../providers/logging/provider.py | 4 +- src/agent_engine/parsers/yaml/parser.py | 22 +++-- src/agent_engine/runtime/hooks/errors.py | 9 +- src/agent_engine/runtime/hooks/manager.py | 7 +- src/agent_engine/runtime/hooks/models.py | 19 ++++ tests/engine/test_engine_flow.py | 42 ++++++++- tests/runtime/hooks/test_hook_manager.py | 26 ++++-- tests/runtime/hooks/test_hooks_schema.py | 12 +++ tests/runtime/test_engine_hooks.py | 87 ++++++++++++++++++- tests/test_logging.py | 19 ++++ 17 files changed, 314 insertions(+), 49 deletions(-) diff --git a/docs/RUNTIME_HOOKS.md b/docs/RUNTIME_HOOKS.md index 44ae26b5..b53491b4 100644 --- a/docs/RUNTIME_HOOKS.md +++ b/docs/RUNTIME_HOOKS.md @@ -109,8 +109,9 @@ hooks: The id is resolved through `plugins/plugins.toml` `[hooks.plugins]`. - **`ref`** *(advanced / backwards compatible)* — explicit import path to the hook callable. Use either `ref` or `plugin` + `method`, never both. -- **`failure_policy`** *(optional, `fail` | `warn`, default `fail`)* — `fail` - aborts the operation (fail-closed); `warn` logs and continues. +- **`failure_policy`** *(optional, `fail` | `warn`, default `fail` — except + `on_tool_error`, which defaults to `warn`)* — `fail` aborts the operation + (fail-closed); `warn` logs and continues. ### Validation @@ -443,7 +444,8 @@ that and rely on env/config-based service credentials for the discovery phase. ## Error policy -Fail-closed by default — security hooks must not be silently skipped: +Fail-closed by default, except `on_tool_error` (see below) — security hooks +must not be silently skipped: | Point | On hook failure (`failure_policy: fail`) | |---|---| @@ -454,15 +456,16 @@ Fail-closed by default — security hooks must not be silently skipped: | `before_tool_call` | the run fails (a policy gate can block the call) | | `after_tool_call` | the run fails | | `transform_tool_result` | the run fails (use `warn` to pass the original result through) | -| `on_tool_error` | the run fails | +| `on_tool_error` | the run fails — but **defaults to `warn`** (see below) | | `before_mcp_request` | the MCP request fails | | `after_mcp_response` | the MCP request fails | | `on_run_error` | logged; **the original run error is preserved** | Use `failure_policy: warn` for best-effort hooks (e.g. audit) that should log -and continue instead of aborting. Hook errors are never silently swallowed: -they are logged with their point and `ref`, and (except `on_engine_stop` and -`on_run_error`, which are best-effort) raised. +and continue instead of aborting — `on_tool_error` already defaults to this. +Hook errors are never silently swallowed: they are logged with their point and +`ref`, and (except `on_engine_stop` and `on_run_error`, which are best-effort) +raised. --- diff --git a/docs/runtime-hooks.mdx b/docs/runtime-hooks.mdx index 758a87bc..e7256f8f 100644 --- a/docs/runtime-hooks.mdx +++ b/docs/runtime-hooks.mdx @@ -170,11 +170,11 @@ The YAML never contains an import path, a config block, or a secret — it only - What happens if the hook itself raises. + What happens if the hook itself raises. Defaults to `fail` everywhere except `on_tool_error`, which defaults to `warn`. - **Default (fail-closed).** The operation is aborted and the request fails. Use for anything security-critical, like auth — a broken auth hook must never fail open. + **Fail-closed.** The operation is aborted and the request fails. Use for anything security-critical, like auth — a broken auth hook must never fail open. The error is logged and the operation continues as if the hook had succeeded. Use for best-effort work like audit logging, where a logging failure shouldn't block the user. diff --git a/examples/config.schema.json b/examples/config.schema.json index e74f1960..b8bbeeb2 100644 --- a/examples/config.schema.json +++ b/examples/config.schema.json @@ -195,7 +195,8 @@ "failure_policy": { "type": "string", "enum": ["fail", "warn"], - "default": "fail" + "default": "fail", + "description": "Defaults to 'fail' (fail-closed) at every hook point except on_tool_error, which defaults to 'warn'." } }, "oneOf": [ diff --git a/src/agent_engine/core/spec.py b/src/agent_engine/core/spec.py index 0b9e02bd..05dc045e 100644 --- a/src/agent_engine/core/spec.py +++ b/src/agent_engine/core/spec.py @@ -2,10 +2,16 @@ from abc import ABC, abstractmethod from dataclasses import dataclass, field +from enum import StrEnum from agent_engine.core.execution import ExecutionPolicy +class FailurePolicy(StrEnum): + FAIL = "fail" + WARN = "warn" + + @dataclass(frozen=True) class BaseModelConfig: provider: str @@ -117,16 +123,14 @@ class GraphNode: class HookSpec: """One declared runtime hook: where it runs, what it is, how it behaves. - ``point`` is a hook lifecycle point (e.g. "before_mcp_request"). A hook is - declared either by explicit Python ``ref`` or by managed ``plugin`` + - ``method`` resolved through plugins.toml. ``failure_policy`` is "fail" - (default, fail-closed) or "warn" (best-effort: log and continue on hook - error). + ``failure_policy``'s ``FAIL`` default applies only when constructed + directly; the YAML parser picks its own default per point instead + (``agent_engine.runtime.hooks.models.DEFAULT_FAILURE_POLICY``). """ point: str ref: str | None = None - failure_policy: str = "fail" + failure_policy: FailurePolicy = FailurePolicy.FAIL plugin: str | None = None method: str | None = None diff --git a/src/agent_engine/engine/langgraph/engine.py b/src/agent_engine/engine/langgraph/engine.py index b2378a42..d1a57e9c 100644 --- a/src/agent_engine/engine/langgraph/engine.py +++ b/src/agent_engine/engine/langgraph/engine.py @@ -383,6 +383,7 @@ def accumulate_tokens(inp: int, out: int) -> None: run_id=ctx.run_id, system=self._system_name, error=type(exc).__name__, + message=exc, ) await self._fail_run(ctx.run_id) await hook_manager.run_run_error(ctx, exc) @@ -645,6 +646,7 @@ async def run_graph() -> None: run_id=ctx.run_id, system=self._system_name, error=type(exc).__name__, + message=exc, ) await self._fail_run(ctx.run_id) await hook_manager.run_run_error(ctx, exc) diff --git a/src/agent_engine/engine/langgraph/nodes.py b/src/agent_engine/engine/langgraph/nodes.py index b86f11a2..bfc39ddf 100644 --- a/src/agent_engine/engine/langgraph/nodes.py +++ b/src/agent_engine/engine/langgraph/nodes.py @@ -33,7 +33,7 @@ run_tool_loop, ) from agent_engine.loaders.resolver_loader import ResolverLoader -from agent_engine.logging_config import log +from agent_engine.logging_config import log, safe_error from agent_engine.runtime.execution import ( ExecutionLimitExceeded, blocked_message, @@ -41,6 +41,7 @@ log_limit, ) from agent_engine.runtime.hooks import ( + HookExecutionError, HookManager, ToolCallContext, ToolRequestContext, @@ -345,7 +346,7 @@ async def _record_error( The failure is returned (not raised) so the model can read it and recover. """ - error = str(exc)[:200] + error = safe_error(exc) used_tools.append(self._usage(call, "failed", error=error)) self._log_call(logging.WARNING, "tool call failed", call, ms=latency_ms, error=error) @@ -582,8 +583,24 @@ async def invoke(message: str) -> str: # to the LangGraph runtime so the checkpoint is taken — it is # control flow, not a child failure. Never swallow it. raise + except HookExecutionError: + # A fail-closed hook (e.g. before_tool_call) rejected this child's + # own call. That must abort the run like any other fail-closed + # hook, not be reported as a recoverable child-agent failure. + raise except Exception as exc: - return f"Agent error: {exc}" + log( + logger, + logging.WARNING, + "child agent call failed", + agent=entry.id, + error=type(exc).__name__, + message=exc, + ) + return ( + f"Agent '{entry.id}' failed to complete this request. " + "This is an internal error, not a problem with the arguments." + ) finally: current_streams.reset(sink_token) @@ -645,7 +662,28 @@ async def invoke_tool(tc: dict[str, Any]) -> str: except ExecutionLimitExceeded as exc: log_limit(exc) return blocked_message(exc) - return cast(str, await tool.ainvoke(tc["args"])) + try: + return cast(str, await tool.ainvoke(tc["args"])) + except GraphInterrupt: + raise + except HookExecutionError: + # A fail-closed hook (e.g. before_tool_call) rejected this call. + # That must abort the run like any other fail-closed hook, not be + # reported as a recoverable child-agent failure. + raise + except Exception as exc: + log( + logger, + logging.WARNING, + "child agent call failed", + agent=tc["name"], + error=type(exc).__name__, + message=exc, + ) + return ( + f"Agent '{tc['name']}' failed to complete this request. " + "This is an internal error, not a problem with the arguments." + ) response = await run_tool_loop(bound_model, messages, state, self._node_path, invoke_tool) answer = as_text(response.content) diff --git a/src/agent_engine/logging_config.py b/src/agent_engine/logging_config.py index fe00437d..f2f9c8bc 100644 --- a/src/agent_engine/logging_config.py +++ b/src/agent_engine/logging_config.py @@ -4,9 +4,31 @@ import logging import os +from pydantic import ValidationError + request_id_var: contextvars.ContextVar[str] = contextvars.ContextVar("request_id", default="") _NOISY = ("httpx", "httpcore", "anthropic", "openai", "urllib3") +_ERROR_LIMIT = 200 + + +def validation_problems(exc: ValidationError) -> str: + """Which fields were rejected and why, without the values that were sent.""" + return "; ".join( + f"{'.'.join(str(part) for part in err['loc']) or 'input'}: {err['msg']}" + for err in exc.errors() + ) + + +def safe_error(exc: BaseException, limit: int = _ERROR_LIMIT) -> str: + """An exception rendered for a log field: bounded, single-line, no payload. + + ``str()`` on a pydantic error appends the ``input_value`` it rejected — the + caller's own arguments — so validation failures are reduced to their field + reasons instead. + """ + detail = validation_problems(exc) if isinstance(exc, ValidationError) else str(exc) + return " ".join(detail.split())[:limit] def _kv(key: str, value: object) -> str: @@ -29,7 +51,11 @@ def format(self, record: logging.LogRecord) -> str: def log(logger: logging.Logger, level: int, event: str, **fields: object) -> None: - logger.log(level, event, extra={"fields": fields}) + """Emit one structured line. Exception values are rendered by ``safe_error``, + so no call site can leak a rejected payload into a log field by passing the + exception straight through.""" + safe = {k: safe_error(v) if isinstance(v, BaseException) else v for k, v in fields.items()} + logger.log(level, event, extra={"fields": safe}) def configure_logging(level: str | None = None) -> None: diff --git a/src/agent_engine/observability/providers/logging/provider.py b/src/agent_engine/observability/providers/logging/provider.py index f7b83b18..809a9628 100644 --- a/src/agent_engine/observability/providers/logging/provider.py +++ b/src/agent_engine/observability/providers/logging/provider.py @@ -26,7 +26,7 @@ def on_llm_end(self, response: Any, **kwargs: Any) -> None: log(logger, logging.INFO, "llm end", tokens=usage) def on_llm_error(self, error: BaseException, **kwargs: Any) -> None: - log(logger, logging.ERROR, "llm error", error=str(error)) + log(logger, logging.ERROR, "llm error", error=error) def on_tool_start(self, serialized: dict[str, Any], input_str: str, **kwargs: Any) -> None: log(logger, logging.INFO, "tool start", name=(serialized or {}).get("name", "?")) @@ -37,7 +37,7 @@ def on_tool_end(self, output: Any, **kwargs: Any) -> None: log(logger, logging.DEBUG, "tool output", value=str(output)[:300]) def on_tool_error(self, error: BaseException, **kwargs: Any) -> None: - log(logger, logging.WARNING, "tool end", status="error", error=str(error)) + log(logger, logging.WARNING, "tool end", status="error", error=error) def on_chain_start( self, serialized: dict[str, Any], inputs: dict[str, Any], **kwargs: Any diff --git a/src/agent_engine/parsers/yaml/parser.py b/src/agent_engine/parsers/yaml/parser.py index 13afbfe1..24d12fef 100644 --- a/src/agent_engine/parsers/yaml/parser.py +++ b/src/agent_engine/parsers/yaml/parser.py @@ -14,6 +14,7 @@ BaseModelConfig, BasePromptSet, DefaultsConfig, + FailurePolicy, GraphNode, HooksConfig, HookSpec, @@ -30,7 +31,7 @@ ) from agent_engine.parsers.errors import ParseError from agent_engine.parsers.parser import Parser -from agent_engine.runtime.hooks.models import HOOK_POINTS +from agent_engine.runtime.hooks.models import DEFAULT_FAILURE_POLICY, HOOK_POINTS _SECRET_MARKERS = ("api_key", "apikey", "secret", "token", "password", "private_key") _SECRET_KEY_EXEMPTIONS = {"max_tokens"} @@ -122,7 +123,6 @@ def _validate_unknown_keys( ) - def _validate_plugins(plugins: Any, errors: list[ValidationError]) -> None: if plugins is None: return @@ -218,7 +218,9 @@ def _build_hooks(raw: Any) -> HooksConfig: HookSpec( point=point, ref=entry.get("ref"), - failure_policy=entry.get("failure_policy", "fail"), + failure_policy=FailurePolicy( + entry.get("failure_policy", DEFAULT_FAILURE_POLICY[point]) + ), plugin=entry.get("plugin"), method=entry.get("method"), ) @@ -320,9 +322,10 @@ def _validate_hook_entry(point: str, index: int, entry: Any, errors: list[Valida "Removed field; hook configuration belongs in hook/plugin code", ) ) - policy = entry.get("failure_policy", "fail") - if policy not in ("fail", "warn"): - errors.append(ValidationError(f"{base}.failure_policy", "Must be 'fail' or 'warn'")) + policy = entry.get("failure_policy", FailurePolicy.FAIL.value) + options = [p.value for p in FailurePolicy] + if policy not in options: + errors.append(ValidationError(f"{base}.failure_policy", f"Must be one of {options}")) def _validate_hooks(hooks: Any, errors: list[ValidationError]) -> None: @@ -415,7 +418,6 @@ def _validate_mcps(mcps: dict[str, Any], errors: list[ValidationError]) -> None: _validate_mcp_tool_tags(mcp_id, raw, errors) - def _validate_model( path: str, raw: Any, errors: list[ValidationError], is_fallback: bool = False ) -> None: @@ -424,10 +426,7 @@ def _validate_model( if not isinstance(raw, dict): errors.append(ValidationError(path, "Must be a mapping")) return - _validate_unknown_keys( - path, raw, _FALLBACK_MODEL_KEYS if is_fallback else _MODEL_KEYS, errors - ) - + _validate_unknown_keys(path, raw, _FALLBACK_MODEL_KEYS if is_fallback else _MODEL_KEYS, errors) provider = raw.get("provider") if not isinstance(provider, str) or not provider.strip(): @@ -588,7 +587,6 @@ def _validate(self, data: dict[str, Any]) -> list[ValidationError]: return errors - def _validate_graph( self, graph: Any, diff --git a/src/agent_engine/runtime/hooks/errors.py b/src/agent_engine/runtime/hooks/errors.py index 61fc508f..33d61a3f 100644 --- a/src/agent_engine/runtime/hooks/errors.py +++ b/src/agent_engine/runtime/hooks/errors.py @@ -27,10 +27,15 @@ def __init__(self, point: str, ref: str, reason: str) -> None: class HookExecutionError(HookError): - """A hook raised while running. Wraps the original exception as ``__cause__``.""" + """A hook raised while running. Wraps the original exception as ``__cause__``. + + The message names only the failing hook's point/ref and the cause's type — + never the cause's own message, which may carry payload or credential + material the hook was handling. ``str(exc)`` is safe to log anywhere. + """ def __init__(self, point: str, ref: str, cause: BaseException) -> None: self.point = point self.ref = ref self.cause = cause - super().__init__(f"Hook for '{point}' (ref='{ref}') failed: {cause}") + super().__init__(f"Hook for '{point}' (ref='{ref}') failed: {type(cause).__name__}") diff --git a/src/agent_engine/runtime/hooks/manager.py b/src/agent_engine/runtime/hooks/manager.py index 07b2bb12..0aa0df61 100644 --- a/src/agent_engine/runtime/hooks/manager.py +++ b/src/agent_engine/runtime/hooks/manager.py @@ -18,7 +18,7 @@ for best-effort audit hooks). * ``transform_tool_result`` fail-> the run fails (use ``failure_policy: warn`` to pass the original, untransformed result through instead). - * ``on_tool_error`` failure -> the run fails (use ``failure_policy: warn``). + * ``on_tool_error`` failure -> logged and skipped (default ``warn`` here). * ``before_mcp_request`` fail -> the MCP request fails. * ``after_mcp_response`` fail -> the MCP request fails (observe-only payload). * ``on_run_error`` failure -> logged; the original run error is preserved. @@ -38,6 +38,7 @@ from pathlib import Path from typing import Any +from agent_engine.core.spec import FailurePolicy from agent_engine.runtime.hooks.errors import HookExecutionError, HookLoadError from agent_engine.runtime.hooks.loader import HookLoader from agent_engine.runtime.hooks.models import ( @@ -65,7 +66,7 @@ class LoadedHook: point: HookPoint ref: str func: Any - failure_policy: str = "fail" + failure_policy: FailurePolicy = FailurePolicy.FAIL plugin: str | None = None method: str | None = None event_mode: bool = False @@ -354,7 +355,7 @@ async def _invoke( hook.failure_policy, type(exc).__name__, ) - if hook.failure_policy == "warn": + if hook.failure_policy == FailurePolicy.WARN: return None raise HookExecutionError(hook.point, hook.ref, exc) from exc return result diff --git a/src/agent_engine/runtime/hooks/models.py b/src/agent_engine/runtime/hooks/models.py index 8c361551..b71e9e9e 100644 --- a/src/agent_engine/runtime/hooks/models.py +++ b/src/agent_engine/runtime/hooks/models.py @@ -13,6 +13,8 @@ from dataclasses import dataclass, field from typing import Any, Literal, TypeVar +from agent_engine.core.spec import FailurePolicy + # The supported lifecycle points. Adding a point here is the one place that # enables a new hook kind across schema validation, loading, and execution. HookPoint = Literal[ @@ -43,6 +45,23 @@ "after_mcp_response", ) +DEFAULT_FAILURE_POLICY: dict[HookPoint, FailurePolicy] = { + "on_engine_start": FailurePolicy.FAIL, + "on_engine_stop": FailurePolicy.FAIL, + "on_run_start": FailurePolicy.FAIL, + "on_run_end": FailurePolicy.FAIL, + "on_run_error": FailurePolicy.FAIL, + "before_tool_call": FailurePolicy.FAIL, + "after_tool_call": FailurePolicy.FAIL, + "transform_tool_result": FailurePolicy.FAIL, + "on_tool_error": FailurePolicy.WARN, + "before_mcp_request": FailurePolicy.FAIL, + "after_mcp_response": FailurePolicy.FAIL, +} +assert set(DEFAULT_FAILURE_POLICY) == set(HOOK_POINTS), ( + "DEFAULT_FAILURE_POLICY must declare a default for every HOOK_POINTS entry" +) + T = TypeVar("T") diff --git a/tests/engine/test_engine_flow.py b/tests/engine/test_engine_flow.py index 635d3e5a..891cb979 100644 --- a/tests/engine/test_engine_flow.py +++ b/tests/engine/test_engine_flow.py @@ -14,6 +14,7 @@ from __future__ import annotations +import logging from collections.abc import AsyncIterator, Callable from dataclasses import dataclass from pathlib import Path @@ -97,6 +98,15 @@ async def astream(self, messages: list[Any]) -> AsyncIterator[AIMessage]: raise RuntimeError("Model stream failed") +# Tool results the fake models were handed back, for asserting what a failure +# actually puts into the model's context. +SEEN_TOOL_RESULTS: list[str] = [] + + +def _record_tool_results(messages: list[Any]) -> None: + SEEN_TOOL_RESULTS.extend(str(m.content) for m in messages if isinstance(m, ToolMessage)) + + class FakeChatModel: """A scriptless stand-in: route through one tool, then answer.""" @@ -121,6 +131,7 @@ async def astream(self, messages: list[Any]) -> AsyncIterator[AIMessage]: yield self._respond(messages) def _respond(self, messages: list[Any]) -> AIMessage: + _record_tool_results(messages) already_called = any(isinstance(m, ToolMessage) for m in messages) if self._tool_names and not already_called: call = ToolCall( @@ -171,6 +182,7 @@ def agent( tools: tuple[ToolSpec, ...] = (), protected: bool = False, auto_mode: bool = True, + model: ModelConfig = _MODEL, ) -> GraphNode: # These flow tests exercise routing/tool execution, not Human-in-the-Loop, so # they default to auto_mode=True (no approval interrupts) — the behavior an @@ -180,7 +192,7 @@ def agent( id=node_id, name=node_id, description=f"{node_id} agent", - model=_MODEL, + model=model, protected=protected, prompts=BasePromptSet(), tools=tools, @@ -189,12 +201,14 @@ def agent( return GraphNode(node=spec) -def orchestrator(node_id: str, children: list[GraphNode]) -> GraphNode: +def orchestrator( + node_id: str, children: list[GraphNode], *, model: ModelConfig = _MODEL +) -> GraphNode: spec = OrchestratorSpec( id=node_id, name=node_id, description=f"{node_id} orchestrator", - model=_MODEL, + model=model, prompts=OrchestratorPromptSet(), ) return GraphNode(node=spec, children=tuple(children)) @@ -272,6 +286,28 @@ async def test_orchestrator_routes_to_matching_child(tmp_path: Path, model_facto assert result.visited == ["root", "root/super"] +async def test_orchestrator_recovers_from_child_agent_crash( + tmp_path: Path, model_factory: Any, caplog: pytest.LogCaptureFixture +) -> None: + # Regression: a child agent's own run raising (its model is down) must not + # crash the whole orchestrator run — the orchestrator still answers, and + # the model gets a generic notice instead of the run dying with a 500. + SEEN_TOOL_RESULTS.clear() + down = ModelConfig(provider="fake", name="failing-primary", temperature=None) + spec = system(orchestrator("root", [agent("flights", model=down)])) + + with caplog.at_level(logging.WARNING): + result = await run_message(spec, tmp_path, model_factory, "please handle flights") + + assert result.answer == "ok" + returned = "\n".join(SEEN_TOOL_RESULTS) + assert "Agent 'flights' failed to complete this request." in returned + assert "not a problem with the arguments" in returned + + record = next(r for r in caplog.records if r.getMessage() == "child agent call failed") + assert getattr(record, "fields", {})["error"] == "RuntimeError" + + async def test_nested_tool_usage_is_recorded(tmp_path: Path, model_factory: Any) -> None: # Regression for the supervisor used_tools merge: a tool called by a nested # agent must surface in the top-level trace. diff --git a/tests/runtime/hooks/test_hook_manager.py b/tests/runtime/hooks/test_hook_manager.py index fea0c8fe..6d530339 100644 --- a/tests/runtime/hooks/test_hook_manager.py +++ b/tests/runtime/hooks/test_hook_manager.py @@ -7,7 +7,7 @@ import pytest -from agent_engine.core.spec import HooksConfig, HookSpec +from agent_engine.core.spec import FailurePolicy, HooksConfig, HookSpec from agent_engine.runtime.hooks.errors import HookExecutionError from agent_engine.runtime.hooks.manager import HookManager from agent_engine.runtime.hooks.models import ( @@ -297,8 +297,20 @@ async def test_hook_failure_raises_with_point_and_ref() -> None: assert isinstance(exc.value.cause, RuntimeError) +async def test_hook_execution_error_str_omits_cause_message() -> None: + # str(HookExecutionError) is logged and may reach an API error response — it + # must never repeat the wrapped cause's own message, which can carry secrets. + mgr = _manager(HookSpec("after_tool_call", f"{_FIX}:secret_boom")) + with pytest.raises(HookExecutionError) as exc: + await mgr.run_after_tool_call( + RunContext(), ToolCallContext(agent_id="a", tool_name="t", provider="local") + ) + assert "secret-context7-key" not in str(exc.value) + assert "RuntimeError" in str(exc.value) + + async def test_failure_policy_warn_does_not_raise() -> None: - mgr = _manager(HookSpec("after_tool_call", f"{_FIX}:boom", failure_policy="warn")) + mgr = _manager(HookSpec("after_tool_call", f"{_FIX}:boom", failure_policy=FailurePolicy.WARN)) # Should swallow the error and continue. await mgr.run_after_tool_call( RunContext(), ToolCallContext(agent_id="a", tool_name="t", provider="local") @@ -313,7 +325,7 @@ async def test_managed_hook_failure_policy_warn_does_not_raise(tmp_path: Path) - "after_tool_call", plugin="managed", method="audit_warn", - failure_policy="warn", + failure_policy=FailurePolicy.WARN, ), ) ), @@ -373,7 +385,9 @@ async def test_changed_hook_payload_logs_safe_applied_event( async def test_hook_failure_log_omits_exception_message( caplog: pytest.LogCaptureFixture, ) -> None: - mgr = _manager(HookSpec("before_mcp_request", f"{_FIX}:secret_boom", failure_policy="warn")) + mgr = _manager( + HookSpec("before_mcp_request", f"{_FIX}:secret_boom", failure_policy=FailurePolicy.WARN) + ) caplog.clear() with caplog.at_level(logging.ERROR, logger="agent_engine.runtime.hooks.manager"): @@ -405,7 +419,9 @@ async def test_transform_tool_result_returns_modified_result() -> None: async def test_transform_tool_result_warn_failure_keeps_original() -> None: # A failing transform under failure_policy=warn must not alter the result. - mgr = _manager(HookSpec("transform_tool_result", f"{_FIX}:boom", failure_policy="warn")) + mgr = _manager( + HookSpec("transform_tool_result", f"{_FIX}:boom", failure_policy=FailurePolicy.WARN) + ) original = ToolResultContext("a", "t", "mcp", result="keep-me") out = await mgr.run_transform_tool_result(None, original) assert out is original diff --git a/tests/runtime/hooks/test_hooks_schema.py b/tests/runtime/hooks/test_hooks_schema.py index 11caa94d..6c70b856 100644 --- a/tests/runtime/hooks/test_hooks_schema.py +++ b/tests/runtime/hooks/test_hooks_schema.py @@ -135,6 +135,18 @@ def test_invalid_failure_policy_fails(tmp_path: Path) -> None: assert "failure_policy" in str(exc.value) +def test_on_tool_error_defaults_to_warn_other_points_default_to_fail(tmp_path: Path) -> None: + spec = _parse( + tmp_path, + "hooks:\n" + " on_tool_error:\n - ref: m:toolerr\n" + " before_tool_call:\n - ref: m:before\n", + ) + by_point = {h.point: h.failure_policy for h in spec.hooks.hooks} # type: ignore[attr-defined] + assert by_point["on_tool_error"] == "warn" + assert by_point["before_tool_call"] == "fail" + + def test_secret_like_hook_value_rejected(tmp_path: Path) -> None: # The existing secret scanner also covers the hooks section: no inline tokens. with pytest.raises(ParseError): diff --git a/tests/runtime/test_engine_hooks.py b/tests/runtime/test_engine_hooks.py index 9568aa75..797de93e 100644 --- a/tests/runtime/test_engine_hooks.py +++ b/tests/runtime/test_engine_hooks.py @@ -18,17 +18,20 @@ from agent_engine.core.spec import ( AgentSpec, BasePromptSet, + FailurePolicy, GraphNode, HooksConfig, HookSpec, ModelConfig, + OrchestratorPromptSet, + OrchestratorSpec, SystemMeta, SystemSpec, ToolSpec, ) from agent_engine.engine.langgraph.engine import LangGraphEngine from agent_engine.parsers.yaml.parser import YAMLParser -from agent_engine.runtime.hooks.errors import HookLoadError +from agent_engine.runtime.hooks.errors import HookExecutionError, HookLoadError from agent_engine.runtime.hooks.models import RunContext from tests.runtime.hooks import fixtures @@ -89,6 +92,19 @@ def _agent(node_id: str, *, tools: tuple[ToolSpec, ...] = ()) -> GraphNode: ) +def _orchestrator(node_id: str, children: list[GraphNode]) -> GraphNode: + return GraphNode( + node=OrchestratorSpec( + id=node_id, + name=node_id, + description=f"{node_id} orchestrator", + model=_MODEL, + prompts=OrchestratorPromptSet(), + ), + children=tuple(children), + ) + + def _system(graph: GraphNode, *hooks: HookSpec) -> SystemSpec: return SystemSpec( meta=SystemMeta(name="hooks-system"), @@ -111,6 +127,15 @@ def _write_tool(base_dir: Path, tool_id: str) -> None: ) +def _write_failing_tool(base_dir: Path, tool_id: str) -> None: + tools_dir = base_dir / "plugins" / "tools" + tools_dir.mkdir(parents=True, exist_ok=True) + (tools_dir / f"{tool_id}.py").write_text( + f"def {tool_id}(message: str) -> str:\n raise RuntimeError('tool exploded')\n", + encoding="utf-8", + ) + + async def test_on_engine_start_runs_during_build(tmp_path: Path, model_factory: Any) -> None: spec = _system(_agent("solo"), HookSpec("on_engine_start", f"{_FIX}:record_engine_start")) async with LangGraphEngine(tmp_path, model_factory=model_factory) as engine: @@ -344,3 +369,63 @@ async def test_on_engine_stop_failure_does_not_block_cleanup( await engine.build(spec) await engine.close() # must not raise despite the failing stop hook assert engine._mcp_tools == {} + + +async def test_on_tool_error_hook_failure_does_not_abort_run_by_default( + tmp_path: Path, model_factory: Any +) -> None: + # Parsed from YAML with no failure_policy declared, so this exercises the + # real default rather than an explicitly-passed one: the tool fails, its + # reporting hook fails too, and the model still answers. + _write_failing_tool(tmp_path, "boom_tool") + config_path = tmp_path / "agents.yml" + config_path.write_text( + "system: {name: hooks-system}\n" + "tools: {boom_tool: {description: boom}}\n" + "agents: {solo: {description: d, auto: true, tools: [boom_tool]}}\n" + "graph: {solo: }\n" + f"hooks: {{on_tool_error: [{{ref: '{_FIX}:boom'}}]}}\n", + encoding="utf-8", + ) + spec = YAMLParser().parse(str(config_path)) + assert [h.failure_policy for h in spec.hooks.hooks] == [FailurePolicy.WARN] + + async with LangGraphEngine(tmp_path, model_factory=model_factory) as engine: + await engine.build(spec) + result = await engine.run("call boom_tool") + + assert result.answer == "ok" + assert result.used_tools[0].status == "failed" + assert [c[0] for c in fixtures.CALLS] == [] # the hook raised, nothing recorded + + +async def test_on_tool_error_hook_failure_with_fail_aborts_run( + tmp_path: Path, model_factory: Any +) -> None: + _write_failing_tool(tmp_path, "boom_tool") + spec = _system( + _agent("solo", tools=(ToolSpec("boom_tool", "boom"),)), + HookSpec("on_tool_error", f"{_FIX}:boom", failure_policy=FailurePolicy.FAIL), + ) + async with LangGraphEngine(tmp_path, model_factory=model_factory) as engine: + await engine.build(spec) + with pytest.raises(HookExecutionError, match="on_tool_error"): + await engine.run("call boom_tool") + + +async def test_fail_closed_before_tool_call_aborts_run_under_orchestrator( + tmp_path: Path, model_factory: Any +) -> None: + # Regression: a child agent called by an orchestrator must not have its + # fail-closed before_tool_call hook silently swallowed by the + # orchestrator's own child-call error handling — that would turn a + # security gate into a fail-open no-op. + _write_tool(tmp_path, "local_tool") + spec = _system( + _orchestrator("root", [_agent("child", tools=(ToolSpec("local_tool", "local"),))]), + HookSpec("before_tool_call", f"{_FIX}:boom"), # default failure_policy=fail + ) + async with LangGraphEngine(tmp_path, model_factory=model_factory) as engine: + await engine.build(spec) + with pytest.raises(HookExecutionError, match="before_tool_call"): + await engine.run("call local_tool") diff --git a/tests/test_logging.py b/tests/test_logging.py index 33d468de..bb5dc1f3 100644 --- a/tests/test_logging.py +++ b/tests/test_logging.py @@ -2,6 +2,9 @@ import logging +import pytest +from pydantic import BaseModel, ValidationError + from agent_engine.logging_config import ( StructuredFormatter, configure_logging, @@ -94,3 +97,19 @@ def test_begin_request_sanitizes_untrusted_id(): assert _begin_request("bad id\nINFO fake") == "badidINFOfake" minted = _begin_request("!@#$%") assert minted and " " not in minted + + +def test_log_helper_renders_exception_fields_without_payload(caplog): + class _Args(BaseModel): + message: str + + with pytest.raises(ValidationError) as excinfo: + _Args(message={"secret": "hunter2"}) # type: ignore[arg-type] + + with caplog.at_level(logging.WARNING, logger="agent_engine.test"): + log(logging.getLogger("agent_engine.test"), logging.WARNING, "boom", error=excinfo.value) + + rendered = caplog.records[-1].fields["error"] + assert "hunter2" not in rendered # the rejected input never reaches a log field + assert "message:" in rendered # but which field failed still does + assert "\n" not in rendered