Restore tool spans for failed argument validation - #6601

Review summary

Reviewed the diff for restoring the v1.94.0 behavior of recording a tool span for failed argument validation, while keeping wrap_tool_execute as the single wrapper around the full tool execution lifecycle.

What the PR does well

• Correct hook traversal — _record_tool_validation_error in tool_manager.py correctly walks CombinedCapability → WrapperCapability → Instrumentation, respects defer_loading + available_capability_ids, and deduplicates via seen: set[int]. Matches the existing _ctx_for_available_cap style used elsewhere.
• Targeted span — Instrumentation._record_tool_validation_error only fires when validation actually failed (validated.args_valid is False), avoiding double spans for valid calls.
• Doc updates — abstract.py docstring and capabilities.md table/diagram now describe the new wrap_tool_execute lifecycle accurately. The error-hooks section explicitly calls out tool execution as the exception to the general ordering.
• Comprehensive tests — tests/test_logfire.py covers schema/validator/unknown failures, include_content on/off, wrapped_instrumentation on/off, and the deferred-loading interaction. tests/test_capabilities.py adds dedicated lifecycle ordering, onion-order preservation, and recovery tests.
• Backward compatibility — wrap_validation_errors=False callers still get the raw exception path they expect; no behavior change for them.

Notes (not blocking)

The docstring of Instrumentation._record_tool_validation_error says "zero-width tool span" and the PR description uses "zero-width error span", but the implementation creates a regular start_as_current_span (with measurable duration). In the issue thread this was discussed as one of the alternatives to the full span; the author chose the full span. Either the docstring/PR description should be aligned with the chosen approach, or the implementation should actually be zero-width. Cosmetic, not behavioral.

The pre-existing wart that logfire.msg = "running tool: {tool_name}" is reused for validation-error spans (where no tool actually ran) is unchanged by this PR — it was the v1.94.0 behavior too, so restoring it is consistent.

Verdict

No HIGH or CRITICAL findings. Implementation is correct, tests are thorough, and the documented wrap_tool_execute lifecycle change matches the new code path.

Clean fix for #6555. The new _instrument_tool_call closure on ToolManager correctly wraps the full validate+execute lifecycle, restoring the pre-v1.95 span boundary around validation failures. Verified: validation errors (schema and validator-rejected args) now produce an execute_tool span with status ERROR and the retry prompt as gen_ai.tool.call.result. Deferred Instrumentation (wrapped via WrapperCapability) is gated on ctx.available_capability_ids and only traces when loaded. User wrap_tool_execute still wraps the complete execute lifecycle (before, body, error, after); on_tool_execute_error still triggers on tool-body exceptions; control-flow exceptions still propagate. Instrumentation no longer overrides wrap_tool_execute, so the OTel span is the outermost wrapper. Tests cover all three scenarios and docs reflect the new semantics.

Verified the fix restores the pre-v1.95 tool-span boundary: Instrumentation._wrap_tool_call now wraps the operation that either raises a stored validation error or executes the tool, and _execute_tool_call_impl is invoked inside the span. wrap_tool_execute semantics correctly expand to enclose before_tool_execute / on_tool_execute_error / after_tool_execute, matching the new docstring and the test_wrap_tool_execute_wraps_execute_lifecycle and test_tool_span_wraps_recovered_and_modified_lifecycle assertions. WrapperCapability and CombinedCapability delegate the new _wrap_tool_call hook symmetrically with the other lifecycle hooks. ToolRetryError raised from a failed validation is recorded as the tool-result attribute and the span is marked ERROR (handle_tool_control_flow=True), matching v1.94. No findings.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e10408b51d

Request changes — unresolved instrumentation/skip interaction

The prior UNRESOLVED review from chatgpt-codex-connector (commit-referenced diff on tool_manager.py) still applies to the current code: when Instrumentation is enabled (auto-prepended as the outermost capability — see pydantic_ai_slim/pydantic_ai/agent/__init__.py:1335) and an inner capability's wrap_tool_execute raises SkipToolExecution, the exception is caught by Instrumentation._run_tool_span's except BaseException block (pydantic_ai_slim/pydantic_ai/capabilities/instrumentation.py:381), which records the span with StatusCode.ERROR and re-raises. The same exception is then caught at tool_manager.py:517 and converted into a successful e.result. Net effect: a tool the agent deliberately skipped produces ERROR-status telemetry — the opposite of the PR's intent (#6555 was about restoring correct tool-span coverage for failed validation, not about regressing successful-skip telemetry).

To verify locally, run the new test_skip_tool_execution test (tests/test_capabilities.py:5647) with Instrumentation added to the agent's capabilities= list and assert logfire.level_num < 17 on the resulting tool span — it will fail under the current code.

The proposed fix is to special-case SkipToolExecution inside Instrumentation._run_tool_span (treating it as control flow, like the existing (CallDeferred, ApprovalRequired) branch on lines 354-372). The companion test (tests/test_logfire.py) should be extended to cover both phases (before and wrap) alongside Instrumentation so this can't regress.

The rest of the PR — type-signature change on AbstractCapability.wrap_tool_execute, removal of the private _instrument_tool_call boundary, decorator-style Hooks validation-failure filtering at hooks.py:1096, and the new tests/test_logfire.py validation-failure span tests — looks correct. The public-API compatibility note in the PR description adequately flags the breaking change for AbstractCapability.wrap_tool_execute overrides, so once the instrumentation/skip interaction is fixed this should be ready.

REQUEST_CHANGES — re-reviewing the diff against the previous unresolved thread.

The one substantive blocker (HIGH severity) is already flagged by chatgpt-codex-connector at pydantic_ai_slim/pydantic_ai/tool_manager.py:521: the new outer wrap_tool_execute boundary routes SkipToolExecution through Instrumentation._run_tool_span, where the except BaseException branch (instrumentation.py:387) marks the span ERROR and records the exception, even though execute_tool_call immediately converts that same exception into a successful replacement result. I traced the path end-to-end (the CombinedCapability chain places Instrumentation as the outermost cap per CapabilityOrdering(position=outermost)), and the bug reproduces as described — telemetry for skipped/denied tools reports error status and logs the SkipToolExecution as an escaped exception.

Per the review conventions I am not duplicating that finding inline; the existing thread is the right place for it. The PR remains blocked until SkipToolExecution (and by extension CallDeferred/ApprovalRequired already handled) is either special-cased in _run_tool_span so the span closes cleanly, or the wrap_tool_execute boundary is restructured to keep the skipped path outside the instrumented action.

The rest of the diff looks sound on a full-file re-read:

• AbstractCapability.wrap_tool_execute defaults to return await handler(args) — args=None correctly raises the stored validation_error via _execute_tool_call_impl.
• hooks.py short-circuits on tool_def is None or args is None, preserving the documented hooks.on.tool_execute valid-call-only contract.
• WrapperCapability and CombinedCapability correctly thread the optional types.
• No double-counting of ctx.usage.tool_calls: _execute_tool_call_impl catches SkipToolExecution from before_tool_execute/innermost handler; execute_tool_call only catches it from cap.wrap_tool_execute itself.
• New tests (test_tool_argument_validation_error_emits_span, test_wrapped_deferred_instrumentation_only_traces_when_loaded, test_tool_span_wraps_recovered_and_modified_lifecycle, test_wrap_tool_execute_wraps_execute_lifecycle, parametrized test_skip_tool_execution) correctly cover schema/validator/unknown failure sources, deferred loading, and lifecycle ordering.

One small doc nit (LOW): the VerboseLogging example in docs/capabilities/custom.md still does print(f" Tool call: {call.tool_name}({args})") against the new args: dict[str, Any] | None — when wrap_tool_execute is invoked for a stored validation failure this will print name(None). Worth a quick update alongside the rest of the docs to avoid surprising copy-paste users.

Reviewed at 25a08edda9494e3cbd94d5666edc919ed12800a8.

No new findings — the surviving concerns from prior review threads (SkipToolExecution span handling, deferral scope, redaction asymmetry between validation and execution spans, exception.escaped bool/string alignment, from __future__ import annotations in the new helper, and the wrap_tool_validate signature) are all addressed at the current head. The implementation correctly restores the v1.94 tool-span behavior for failed argument validation, scopes the include_content=False redaction to the validation path with an explicit rationale comment, and shares retry-prompt construction so telemetry matches the prompt sent to the model. Tests cover both failure_source × include_content paths and the recovery-no-error-span contract for both capability orderings.

Generated by 🔎 CI Review

Reviewed at 4bd0e0339cde7be99a7edaf9ec0bd1533af910e2.

The head commit is a small, additive polish on top of an already-thoroughly-reviewed change: it adds pydantic_ai.tool.failure_stage='validation' and the invalid tool call: <name> logfire.msg to the failed-validation span so backends can distinguish it from execution failures, with matching test assertions on both the failure span and the subsequent successful retry. No new behavior, no new public API, no risk to existing telemetry shape. Local pytest tests/test_logfire.py and pyright on the touched files are clean; the prior resolved threads (redaction under include_content=False, exception.escaped boolean alignment, from __future__ import annotations in _tool_retry.py) all still hold against this commit.

Generated by 🔎 CI Review

Reviewed at bc8538583def4827f36ca039e20d606a6bb305d5.

The PR restores the v1.94.0 failed-validation tool-span contract cleanly: shared retry-prompt construction (pydantic_ai/_tool_retry.py) keeps telemetry aligned with the prompt the model receives; recovery capabilities run before Instrumentation.on_tool_validate_error via CombinedCapability's reverse-order loop; and include_content=False redaction omits the rejected values. Tests cover both schema and validator failure sources, both capability orderings, and raw-mode dispatch. No new findings.

Generated by 🔎 CI Review

Reviewed at 61a11ea852512ee5c117f8c66912eb1cf57ecac8.

This is a well-scoped fix for the regression tracked as #6555. Key observations:

• The shared retry_prompt_from_error helper in the new _retry_prompt.py module is now the single source of retry-prompt text used by _output.py, tool_manager.py, and capabilities/instrumentation.py, so the span's recorded prompt and the message sent to the model cannot drift apart again.
• Instrumentation.on_tool_validate_error correctly emits a zero-width ERROR span only when no recovery capability returns validated args; the recovered path produces no error span (test_recovered_tool_validation_emits_no_error_span).
• The include_content=False branch records exception.type only (with a boolean exception.escaped and a comment explaining why the validation path differs from the execution path), and the assert 'private-invalid-value' not in str(...) line pins that redaction.
• Both ValidationError (schema) and ModelRetry (validator) failure sources are parametrized in test_tool_argument_validation_failure_emits_span; test_raw_mode_validation_failure_emits_span covers handle_call(wrap_validation_errors=False) from a sandboxed dispatcher.

The deliberate scope limit on the redaction asymmetry (validation redaction only, execution redaction deferred) is explicitly called out in the code comment, so future readers won't accidentally widen it without intent.

No new findings.

Generated by 🔎 CI Review