[ColtenOuO]

Summary

Follow up: #71445

HookToolset.call_tool had no error handling — any exception raised by the wrapped hook method (bad arguments from the LLM, a transient network error, etc.) propagated straight up and aborted the whole agent run. Every other hand-written toolset in this provider (SQLToolset, DataFusionToolset, SandboxToolset) already catches tool failures and re-raises them as
pydantic_ai.exceptions.ModelRetry, so the model can read the error and retry within the run instead of failing the task outright.

This brings HookToolset in line with the other toolsets, bounded by the tool's existing max_retries.

Change

• HookToolset.call_tool (toolsets/hook.py) now wraps the hook method call in try/except Exception, re-raising as pydantic_ai.exceptions.ModelRetry with the tool name and original error message.
• Added a short note to the class docstring describing this retry behavior, matching the wording already used by SQLToolset / SandboxToolset.
• Added test_wraps_failure_in_model_retry (tests/.../test_hook.py), using a new _FakeHook.failing_method fixture that always raises.

---

Was generative AI tooling used to co-author this PR?

No

[potiuk]

The change itself is small and the test does fail without it, but it reverses a behaviour the provider documents as deliberate, and the blanket catch has a concrete hazard specific to this toolset. It also no longer applies to main.

The docs document the current behaviour as intentional

From providers/common/ai/docs/choosing_a_toolset.rst, under "What it cannot do" for HookToolset:

It returns exactly one shape. Every result goes through serialize_for_llm
and comes back as a JSON-encoded string; there is no structured error type and
no ModelRetry wrapper, so a hook exception fails the agent run, and the
task with it, instead of giving the model something it can correct.
SQLToolset, by contrast, hands the database's own error back as a retry.

That is not a description of an oversight — it is a documented contrast between the two toolsets. This PR removes it without touching the bullet, so the docs would contradict the code.

Blanket except Exception vs. the documented failure contract

providers/common/ai/docs/toolsets.rst sorts failures into three buckets and reserves ModelRetry for "the agent rejected the request in a way rephrasing could fix", while transient errors "let it propagate — Airflow's task-level retry. A rephrase does nothing for a 503."

Two specifics:

1. The description says every other hand-written toolset already converts failures to ModelRetry. Only SQLToolset does. DataFusionToolset converts just SQLSafetyError and pattern-matched retryable query errors, and SandboxToolset deliberately re-raises SandboxTerminalError with the comment "Retrying cannot help ... Fail the task so Airflow retries the whole thing".
2. HookToolset is the one toolset that can expose non-idempotent methods — the docs' own "Bad" example lists delete_object and load_string. A hook method that already performed its side effect and then raised now gets re-invoked by the model. This also voids a guarantee the LangChain bridge documents specifically for hooks: that a ValidationError raised inside call_tool "propagates rather than being fed back, so a non-idempotent tool that already ran a side effect is not re-invoked".

SQLToolset is safe to do this in because it is read-only by default. HookToolset cannot know which exposed methods are safe to call twice, which is the reason for the documented difference.

Needs a rebase

#71946 renamed _serialize_for_llm to serialize_for_llm, so the return _serialize_for_llm(result) context line no longer applies.

Smaller observations

• The new docstring copies the first half of SQLToolset's wording but drops the max_retries sentence — the part the description advertises ("bounded by the tool's existing max_retries").
• ctx=MagicMock() in the new test is unspec'd. It matches the existing pattern in the file, so not blocking.
• Once max_retries is exhausted the task sees pydantic-ai's retry-exhausted exception rather than the hook's original one, which may change what RetryRule(exception=...) matches in LLMRetryPolicy / ExceptionRetryPolicy. If so that warrants a note under the Changelog header in providers/common/ai/docs/changelog.rst.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. After you've
addressed the points above and pushed an update, an Apache Airflow
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.

[potiuk]

My inclination is to close this rather than iterate on it, but I want the common.ai owners to confirm before I do.

@gopidesupavan @kaxil @Lee-W @ashb — this proposes that HookToolset.call_tool convert every tool failure into ModelRetry, which reverses the behaviour choosing_a_toolset.rst currently documents as a deliberate difference from SQLToolset. My concern is that HookToolset exposes arbitrary hook methods and cannot know which are idempotent, so a method that performed its side effect and then raised would be re-invoked by the model. That seems like a design decision for you rather than something to settle in review.

To be clear about what I think is legitimate here: when the model passes a bad argument, a retry genuinely could fix it, and that is what the three-bucket contract in toolsets.rst means by "the agent rejected the request in a way rephrasing could fix". A narrow version — convert argument-shaped errors, let transient and terminal ones propagate — would fit the contract. That is a different change from this one and deserves its own design discussion.

@ColtenOuO — nothing wrong with the implementation, and thanks for the iteration here and on #71734. If the owners agree with me, the productive next step is an issue proposing the narrow version, which I would be glad to review. If they would rather take this as-is with the docs updated, I will happily defer.

[potiuk]

Hello @ColtenOuO - thank you for your contributions to Apache Airflow!

The Airflow community has introduced a limit of 5 open pull requests at a time for contributors without write access to the repository. You currently have 19 open pull requests, so - as a one-time step of introducing the limit - we closed the ones where maintainers have not engaged yet:

• [AIP-94] Add airflowctl Tasks List Command #70472 - [AIP-94] Add airflowctl Tasks List Command
• Let HookToolset retry a failed tool call instead of failing the task #71853 - Let HookToolset retry a failed tool call instead of failing the task
• Fix scheduler crash when the API server response is a redirect #71962 - Fix scheduler crash when the API server response is a redirect
• Fix manually triggered runs blocking a scheduled run at the same logical_date #72003 - Fix manually triggered runs blocking a scheduled run at the same logical_date
• Fix mismatched XCom values when a dynamically mapped task has a gap #72007 - Fix mismatched XCom values when a dynamically mapped task has a gap
• Refactor schema version checks to share target-branch and worktree logic #72070 - Refactor schema version checks to share target-branch and worktree logic
• Document custom OTel metrics endpoint path support #72073 - Document custom OTel metrics endpoint path support
• Batch XCom lookups when resolving a mapped task's expand() arguments #72107 - Batch XCom lookups when resolving a mapped task's expand() arguments
• Remove redundant full-Dag TaskInstance query from the execution API task-states endpoint #72270 - Remove redundant full-Dag TaskInstance query from the execution API task-states endpoint
• Fix asset dependency graph dropping outlets for tasks that both consume and produce #72272 - Fix asset dependency graph dropping outlets for tasks that both consume and produce
• Make supervisor split-line stdout test actually cover the split in Task SDK #72585 - Make supervisor split-line stdout test actually cover the split in Task SDK
• Fix GET /assets/{asset_id} returning 500 when asset events share a timestamp #72599 - Fix GET /assets/{asset_id} returning 500 when asset events share a timestamp
• Show detailed API error messages in UI toasters #72831 - Show detailed API error messages in UI toasters

These pull requests stay open because maintainers are already engaged in them - they count towards your limit:

• Fix output type loss in AgentOperator's human-in-the-loop review #70132 - Fix output type loss in AgentOperator's human-in-the-loop review
• Decode base64 for bytes-typed HookToolset parameters #70433 - Decode base64 for bytes-typed HookToolset parameters
• Allow providers to ship testing Dags separately from example Dags #70541 - Allow providers to ship testing Dags separately from example Dags
• Reject unknown update_mask fields instead of silently ignoring them #71003 - Reject unknown update_mask fields instead of silently ignoring them
• Speed up clearing downstream tasks on large Dags #71203 - Speed up clearing downstream tasks on large Dags
• Add deadline alert coverage to Dag endpoint tests #72296 - Add deadline alert coverage to Dag endpoint tests

This is not a judgement of you or of your changes. We never told contributors before that opening many pull requests at once was a problem, so there is nothing to feel bad about - and nothing is lost: your branches, commits and the review history stay where they are.

What we ask you to do is to make your first prioritization decision: choose which of the pull requests above matter most to you, and reopen them (up to 5 open at a time, including the ones still open) with the "Reopen pull request" button or gh pr reopen <PR_NUMBER> --repo apache/airflow. Reopen the ones you are ready to follow through - keep them rebased, respond to review comments and fix failing checks.

While your pull requests are waiting for review, the most valuable thing you can do is help in other ways - reviewing other contributors' pull requests, helping with issues, and taking part in the discussions on the devlist and Slack.

Why we introduced the limit, what it means for you and how to reopen or restore a pull request is explained in https://github.com/apache/airflow/blob/main/contributing-docs/32_open_pull_request_limit.rst.

---

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting