fix(engine): stop a failed child-agent call from killing the whole run

[AmitAvital1]

What

An OrchestratorNode child-agent call had no exception handling at all, unlike its sibling AgentNode. An uncaught error (e.g. a pydantic.ValidationError from malformed tool args, or a downed child model) crashed the entire run — a bare HTTP 500 with no assistant reply and no useful log.

This PR fixes that, plus two things found while investigating: hook failure policy was a magic string with no configurability, and exception text was logged raw in several places, leaking payload data (e.g. a validation error's input_value — the caller's own arguments, which may contain credentials).

Changes

• Root fix: OrchestratorNode.invoke_tool and _make_tool.invoke now catch child-agent failures, log them, and return a message so the model/orchestrator can recover — instead of the run dying.
  • GraphInterrupt (HITL control flow) and HookExecutionError from a fail-closed hook (e.g. before_tool_call) are explicitly re-raised: a security gate rejecting a call must still abort the run, not be silently reported as a recoverable child failure.
• Hook failure policy: failure_policy is now a FailurePolicy enum (FAIL/WARN) instead of a magic string, with a per-hook-point default table (DEFAULT_FAILURE_POLICY). on_tool_error now defaults to warn — a broken error-reporting hook shouldn't itself crash an otherwise-recoverable run — while security-critical points (before_tool_call, etc.) stay fail-closed by default. Still fully configurable per hook.
• Safe error logging: a new safe_error()/validation_problems() utility in logging_config.py, now used everywhere an exception is logged (engine run/stream failures, the observability trace callback, tool-call failures). log() itself sanitizes any BaseException value passed as a field. A pydantic ValidationError's str() echoes back the rejected input_value; this reduces it to field names/reasons instead.
• HookExecutionError.__str__ no longer repeats the wrapped cause's own message, for the same reason — its text is safe to log or return in an API error anywhere.
• Docs (RUNTIME_HOOKS.md, runtime-hooks.mdx, config.schema.json) updated for the on_tool_error default change.

Follow-up

A stacked PR on top of this one improves the actual content of the message a failed child-agent call returns to the model (field-level actionable detail instead of a generic notice) — kept separate to keep this fix reviewable on its own.

Test plan

• New regression tests: test_on_tool_error_hook_failure_does_not_abort_run_by_default, test_on_tool_error_hook_failure_with_fail_aborts_run, test_fail_closed_before_tool_call_aborts_run_under_orchestrator, test_orchestrator_recovers_from_child_agent_crash, test_hook_execution_error_str_omits_cause_message, test_log_helper_renders_exception_fields_without_payload
• make lint, make typecheck, make test all pass

🤖 Generated with Claude Code

[Asaf-prog]

Thanks — the core direction makes sense. A failure in an on_tool_error reporting hook should not normally abort a run that is already handling the original tool failure, while security-sensitive hooks should remain fail-closed.

I found one blocking concern with the new run-failure logging:

message=str(exc)

HookExecutionError.__str__() includes the original hook exception message. That means a hook exception containing credentials, request data, or other sensitive context could now be written into the run-level logs.

The hook manager intentionally logs only the exception type, and there is already a test verifying that hook exception messages are not leaked. Logging str(exc) in the engine bypasses that protection at a higher layer.

Please remove the raw exception message from these log fields, or introduce an explicitly sanitized representation that cannot include the wrapped cause text.

Please also add a regression test for the actual behavior this PR changes: an on_tool_error hook using the YAML default should fail without aborting the run, while an explicit failure_policy: fail should still preserve the existing fail-closed behavior.

Once those points are addressed, the overall change looks good.

[AmitAvital1]

Thanks for the catch — fixed at the source rather than in the log call.

Leak fix: HookExecutionError.__str__() was building its message as f"Hook for '{point}' (ref='{ref}') failed: {cause}", embedding the wrapped exception's own message. That's not specific to the new engine log line — anywhere str(exc) on a HookExecutionError is used (including a future routes.py HTTPException(detail=str(exc))) would have the same leak. Changed it to name only the cause's type, matching the discipline HookManager's own logging already follows: f"...failed: {type(cause).__name__}". message=str(exc) in engine.py needed no special-casing after that — it's safe by construction now for every caller.

Regression tests added:

• tests/runtime/hooks/test_hooks_schema.py::test_on_tool_error_defaults_to_warn_other_points_default_to_fail — confirms the YAML default is warn for on_tool_error and fail everywhere else.
• tests/runtime/test_engine_hooks.py::test_on_tool_error_hook_failure_with_warn_does_not_abort_run — a tool fails, its on_tool_error hook also fails under the warn default, and the run still completes normally.
• tests/runtime/test_engine_hooks.py::test_on_tool_error_hook_failure_with_fail_aborts_run — same setup with an explicit failure_policy: fail, and the run still raises (existing fail-closed behavior preserved).
• tests/runtime/hooks/test_hook_manager.py::test_hook_execution_error_str_omits_cause_message — direct regression for the leak, using the existing secret_boom fixture.

All pushed. Full suite + lint + typecheck green.

[AmitAvital1]

One more round based on additional feedback: the model-facing "Agent error: {exc}" string (both where a child's whole run fails, and the malformed-args case fixed earlier) was raw, inconsistent exception text — not something a model can reliably act on — and neither catch site logged anything server-side at all, so for a nested child failure the real detail was completely lost, not just under-detailed.

Changed both to:

• Log "child agent call failed" with error=type(exc).__name__ and message=str(exc) server-side.
• Return a fixed, deterministic "Agent error: '<name>' failed to complete this request." to the model — no exception internals in its context.

Added test_orchestrator_recovers_from_invalid_child_tool_args's log assertion (via caplog) to lock in that the real error is now actually logged. Full suite + lint + typecheck still green.

[AmitAvital1]

Follow-up: the fixed string from the previous commit was safe but not actionable. In the original incident the failure was the model's own malformed call (a dict where _AgentCall wants message: str), and the generic message stripped away the one detail it needed to correct itself and retry.

Schema rejections now return the offending field and expected shape, built from ValidationError.errors() instead of str(exc) — because pydantic's own message appends the rejected input_value, which is the caller's arguments, and in the reported incident held an email and password:

before:  Agent error: 1 validation error for _AgentCall
         message
           Input should be a valid string [type=string_type,
           input_value={'email': 'test@test.com'..., 'username': 'amitrol'}, ...]

after:   Invalid arguments for 'admin_agent': message: Input should be a valid string.
         Correct them and call it again.

Actionable for the model, and its own payload is never handed back to it.

Other failure kinds keep their existing treatment deliberately:

• A tool's own exception is the developer's intentional message to the model ("email already registered") and still passes through — reducing that to a type name would have been a regression.
• An internal child-agent crash stays generic and now says so explicitly ("not a problem with the arguments"), so the model doesn't burn turns retrying arguments that were never at fault.

Same treatment applied to AgentNode._record_error, which had the identical input_value leak into the model's context via f"Tool error: {exc}".

Test asserts all three properties: the run survives, the model receives the actionable field-level reason, and the sentinel argument value never appears in what it was handed.

[AmitAvital1]

Ran a full review pass over this branch. It found a leak I introduced in this PR, plus the original incident's log line still being live. Both fixed; still one squashed commit.

Fixed — credential leak into server logs (introduced here)

The message=str(exc) field I added to make failures debuggable wrote the full pydantic error — input_value and all — into the log, on the exact path this PR exists to fix. Verified before/after:

before:  child agent call failed agent=admin_agent error=ValidationError
         message="1 validation error for _AgentCall
         message
           Input should be a valid string [type=string_type,
           input_value={'email': 'test@test.com', 'password': 'hunter2'}, ...]"

after:   child agent call failed agent=admin_agent error=ValidationError
         message="message: Input should be a valid string"

I had closed the model channel and reopened the same data on the log channel — the _invalid_args_message docstring even warns about it two lines from where I then passed str(exc) to log().

Fixed — observability/providers/logging/provider.py:40

This is verbatim the agent_engine.trace: tool end status=error error="...input_value={'email': ...}" line from the original incident report. It was pre-existing and untouched, and reproduced the leak independently of anything else. Now sanitized.

Both go through one new logging_config.safe_error(), which also bounds every rendering to a single line — the previous multi-line str(exc) broke the structured single-line log format and left unbalanced quotes in a partly model-controlled field.

Test gaps closed

• The warn test passed an explicit policy, so it would have passed against main — it now builds the spec through YAMLParser with no declared policy, exercising the real default. Confirmed it fails when DEFAULT_FAILURE_POLICY is reverted.
• Added test_orchestrator_recovers_from_child_agent_crash — the _make_tool.invoke catch site had zero coverage.
• Added the assertion that would have caught the leak above: the rejected argument value must not appear in any log field. Confirmed it fails against the pre-fix code.
• examples/config.schema.json still declared the old default; updated.

Verified correct, on the record

asyncio.CancelledError is BaseException so except Exception doesn't swallow it; GraphInterrupt re-raise covers every interrupt this codebase actually raises; gating hooks (before_tool_call, before_mcp_request, on_run_start) genuinely still default to fail and no HookSpec construction path bypasses the parser; StrEnum is transparent across f-strings, json.dumps, and == so agentctl diagnostics and the bare-string assertions still hold; ToolRecord.error is already sanitized on both HTTP APIs.

Known, not addressed here

A fail-closed before_tool_call hook fails open for any agent under an orchestrator — its HookExecutionError propagates out of AgentNode and is swallowed by the child-agent catch, so the run completes normally. This is pre-existing and contradicts the documented behavior, but fixing it changes fail-closed semantics repo-wide and deserves its own PR rather than riding along in this one. Happy to open it.

[AmitAvital1]

Ran a fresh review pass against the current state. It found two more real issues — both verified myself, both fixed.

Blocking — the leak fix from last round was incomplete

`AgentNode._record_error` (nodes.py:362, untouched until now) still had `error = str(exc)[:200]` — the exact same class of leak, one call site away from where it was fixed. That raw string flowed into the WARNING log, `ToolUsageRecord.error`, and the `on_tool_error` hook payload delivered to arbitrary plugin code. Verified live with a pydantic model containing a `password` field — leaked before, clean after:

```
before: tool call failed ... error="1 validation error for M
password
Input should be a valid string [type=string_type, input_value={'leaked': 'hunter2'}, ...]"

after: tool call failed ... error="password: Input should be a valid string"
```

Now uses the same `safe_error()` as everywhere else. Also applied it to the two `message=str(exc)` fields in `engine.py` for consistency (not currently exploitable, but no reason to leave one exception-rendering path different from the rest).

Blocking — my own fix widened a fail-open gap

`OrchestratorNode.invoke_tool` had zero try/except on `main` — confirmed via `git show origin/main`. That meant a `HookExecutionError` from a fail-closed `before_tool_call` hook used to propagate uncaught and correctly abort the run. The catch I added to fix the actual bug (malformed tool args) also caught that exception type, silently converting a security gate into a no-op for any child agent called directly by an orchestrator.

Both `invoke_tool` and the sibling `_make_tool.invoke` (which already had this problem pre-existing, now confirmed and not just assumed) now re-raise `HookExecutionError` explicitly before the generic catch, so a fail-closed hook still aborts the run at both call sites.

Added `test_fail_closed_before_tool_call_aborts_run_under_orchestrator` and confirmed it fails without the re-raise (child agent under an orchestrator, hook raises, run used to complete normally — now correctly raises).

Rebased onto `main` (picked up the new YAML unknown-key validation, no conflicts). Lint, typecheck, full suite green.