[PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W opened a new pull request, #72936:
URL: https://github.com/apache/airflow/pull/72936

   The toolsets arrived one at a time, each with a docs section explaining what it does and none explaining when it is the wrong answer. A reader who finds all of them still cannot choose, because the routes overlap: the same target is often reachable through a hook, through a vendor's MCP server, or through a skill, and those answers differ in how credentials are governed, how much surface we have to keep current, and how they fail.
   
   Writing the reasoning down once means the next proposal for a toolset can be answered by reference instead of re-argued.
   
    <!-- SPDX-License-Identifier: Apache-2.0
         https://www.apache.org/licenses/LICENSE-2.0 -->
   
   <!--
   Thank you for contributing!
   
   Please provide above a brief description of the changes made in this pull request.
   Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
   
   Please make sure that your code changes are covered with tests.
   And in case of new features or big changes remember to adjust the documentation.
   
   For user-facing UI changes, please attach before/after screenshots (or a short
   screen recording) so reviewers can assess the visual impact.
   
   Feel free to ping (in general) for the review if you do not see reaction for a few days
   (72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
   
   In case of an existing issue, reference it using one of the following:
   
   * closes: #ISSUE
   * related: #ISSUE
   -->
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   <!--
   If generative AI tooling has been used in the process of authoring this PR, please
   change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
   -->
   
   - [ ] Yes (please specify the tool below)
   
   <!--
   Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
   -->
   
   ---
   
   * Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
   * For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
   * When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   * For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4052562223


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files

Review Comment:
   Both extras are named now. (for datafusion and iceberg)
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4052575008


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files
+from; the catalog resolves the table by name, so the config carries a
+``db_name`` instead, following the same ``DataSourceConfig`` shape that
+``example_analytics.py`` in the ``common.sql`` provider uses:
+
+.. code-block:: python
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="iceberg_default",
+                table_name="users_data",
+                db_name="demo",
+                format="iceberg",
+            ),
+        ],
+        max_rows=100,
+    )
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+Before following this row, check whether the actual need is narrower than that:
+``code_mode=True`` is a flag on ``AgentOperator``, not a toolset on this table
+— it changes how the model invokes the tools it already has, letting it write
+code to call several of them instead of emitting one call per step. It does not
+give the agent somewhere to run arbitrary code of its own, and it avoids the
+``sbx`` backend's production-readiness, network-isolation, and reclamation
+caveats below — but not the reachability one: the glue code still runs in the

Review Comment:
   The reason is now the one `operators/agent.rst` and the `code_mode` docstring give: the generated code runs in Monty's deny-by-default sandbox, and the tools it calls still run in the worker. The conclusion is unchanged — a credential-bearing toolset on the same agent stays within reach whether or not code mode is on.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4045938192


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and

Review Comment:
   `SHOW` is now named as fail-closed once `allowed_tables` is set, with `DESCRIBE`'s different behaviour spelled out — it becomes an ordinary table reference, allowed only when the table it names is on the list.
   The construct enumeration is removed; the page defers to How `allowed_tables` Is Enforced for the rest, which keeps a second list from drifting against the code. The failure-classification bullet now names the two paths that do not raise: `check_query`, which reports `{"valid": false, ...}` itself, and `get_schema`, which returns `{"error": ...}` on an allow-list miss.
   
   I kept only the `SHOW` / `DESCRIBE` split, since the page raises both two sentences earlier and dropping it would leave that hanging.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4045927358


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows

Review Comment:
   the decision-table row and the section now separate object-store formats from a catalog-managed one: parquet, csv and avro take a `uri`, iceberg is looked up by `db_name`, and `datasourceconfig` raises at construction if a catalog format is missing it. added a second, iceberg-shaped example following the config in `example_analytics.py`, so the page no longer offers only the parquet one to copy.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4052568549


##########
providers/common/ai/src/airflow/providers/common/ai/sandbox/sbx.py:
##########
@@ -80,12 +80,18 @@ class SbxSandboxBackend(SandboxBackend):
     backend ships with the provider yet; add one behind :class:`SandboxBackend`
     if you need Kubernetes.
 
-    **Network policy is a host-level setting, not a per-sandbox one.** ``sbx``
-    governs egress through ``sbx policy``, so this backend cannot apply a
-    per-sandbox rule. Rather than let a DAG author believe a
-    :class:`~airflow.providers.common.ai.sandbox.SandboxSpec` restriction is in
-    force when it is not, ``create`` refuses a spec it cannot honor unless the
-    Deployment Manager states the host policy through ``host_network_policy``.
+    **Network policy is layered on a host-level setting, not independent of
+    one.** ``sbx`` governs egress through a host-level ``sbx policy``.
+    ``create`` applies ``allow_egress_to`` as a per-sandbox rule on top of
+    that policy, but the rule can only narrow a host policy that is already
+    ``deny-all`` and never widen one. ``block_network`` has no per-sandbox
+    enforcement at all: no ``sbx`` call implements it. Rather than let a Dag
+    author believe a
+    :class:`~airflow.providers.common.ai.sandbox.SandboxSpec` restriction is
+    in force when it is not, ``create`` raises instead of silently ignoring a
+    spec that names ``allow_egress_to`` or ``block_network``. It lets the

Review Comment:
   The docstring now 
   
   * reuses `:param host_network_policy:` 20 lines down
   * says that `block_network` defaults to `True`, so a bare `SandboxSpec()` already asks for one.
   
   That is the case for `SandboxToolset` when no spec is passed, so it was the wrong one to describe as fine. `toolsets.rst` gains the `block_network` half of the fact it was missing.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["kaxil (via GitHub)" <gi...@apache.org>]

kaxil commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4049263839


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files
+from; the catalog resolves the table by name, so the config carries a
+``db_name`` instead, following the same ``DataSourceConfig`` shape that
+``example_analytics.py`` in the ``common.sql`` provider uses:
+
+.. code-block:: python
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="iceberg_default",
+                table_name="users_data",
+                db_name="demo",
+                format="iceberg",
+            ),
+        ],
+        max_rows=100,
+    )
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.

Review Comment:
   "No tool-level allow-list at all" overstates it. `MCPToolset` subclasses `AbstractToolset` (`toolsets/mcp.py:32`), so it inherits `.filtered(filter_func)`, which returns a `FilteredToolset` that drops non-matching tools from `get_tools`. A Dag author can allow-list MCP tools by name without touching the server. `.prefixed()` and `.filtered()` are siblings on that base class, which makes "`tool_prefix` renames; it does not filter" read as reaching for one and missing the other.
   
   I checked the floor rather than what happens to be installed: `filtered` is on `AbstractToolset` at `pydantic-ai-slim==2.23.0` (`providers/common/ai/pyproject.toml:75`), so no version caveat rescues the absolute. The narrower caveat is still worth making, because the asymmetry with `allowed_methods` is real: `FilteredToolset` overrides `get_tools` only, so it subsets the advertised tool list client-side rather than authorizing anything server-side, and unlike `allowed_methods` (required and non-empty, `toolsets/hook.py:76-77`) it is opt-in. Neither `filtered` nor `FilteredToolset` appears anywhere in this provider's docs today, so this page is where a reader would look for it.



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files
+from; the catalog resolves the table by name, so the config carries a
+``db_name`` instead, following the same ``DataSourceConfig`` shape that
+``example_analytics.py`` in the ``common.sql`` provider uses:
+
+.. code-block:: python
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="iceberg_default",
+                table_name="users_data",
+                db_name="demo",
+                format="iceberg",
+            ),
+        ],
+        max_rows=100,
+    )
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+Before following this row, check whether the actual need is narrower than that:
+``code_mode=True`` is a flag on ``AgentOperator``, not a toolset on this table
+— it changes how the model invokes the tools it already has, letting it write
+code to call several of them instead of emitting one call per step. It does not
+give the agent somewhere to run arbitrary code of its own, and it avoids the
+``sbx`` backend's production-readiness, network-isolation, and reclamation
+caveats below — but not the reachability one: the glue code still runs in the

Review Comment:
   The conclusion holds but the mechanism named here is the one `operators/agent.rst:413-414` explicitly denies: the generated code runs in Monty's deny-by-default sandbox and "cannot read the filesystem, the network, or environment variables". What keeps a credential in reach is the tools rather than the glue. `agent.py:199-201` has wording to borrow: the generated code runs in Monty's sandbox, "the tools it calls still run in the worker, so `code_mode` does not widen what the tools can reach".



##########
providers/common/ai/src/airflow/providers/common/ai/sandbox/sbx.py:
##########
@@ -80,12 +80,18 @@ class SbxSandboxBackend(SandboxBackend):
     backend ships with the provider yet; add one behind :class:`SandboxBackend`
     if you need Kubernetes.
 
-    **Network policy is a host-level setting, not a per-sandbox one.** ``sbx``
-    governs egress through ``sbx policy``, so this backend cannot apply a
-    per-sandbox rule. Rather than let a DAG author believe a
-    :class:`~airflow.providers.common.ai.sandbox.SandboxSpec` restriction is in
-    force when it is not, ``create`` refuses a spec it cannot honor unless the
-    Deployment Manager states the host policy through ``host_network_policy``.
+    **Network policy is layered on a host-level setting, not independent of
+    one.** ``sbx`` governs egress through a host-level ``sbx policy``.
+    ``create`` applies ``allow_egress_to`` as a per-sandbox rule on top of
+    that policy, but the rule can only narrow a host policy that is already
+    ``deny-all`` and never widen one. ``block_network`` has no per-sandbox
+    enforcement at all: no ``sbx`` call implements it. Rather than let a Dag
+    author believe a
+    :class:`~airflow.providers.common.ai.sandbox.SandboxSpec` restriction is
+    in force when it is not, ``create`` raises instead of silently ignoring a
+    spec that names ``allow_egress_to`` or ``block_network``. It lets the

Review Comment:
   The mechanism above is right now, but this predicate reads on what a spec *names* while `_check_spec` gates on what it *asks for*, and `block_network` defaults to `True` (`sandbox/base.py:103`). So `SandboxSpec()` names neither field and still raises (`sbx.py:180`; `test_sbx.py:90-92` asserts exactly that against `host_network_policy="allow-all"`), while `SandboxSpec(block_network=False)` names one and passes under the default `"unknown"` policy (`test_sbx.py:94-102`). That is the direction that surprises a reader: `SandboxToolset` materialises the default spec itself (`toolsets/sandbox.py:196`), so the all-defaults case is the one this sentence says is fine.
   
   The `:param host_network_policy:` entry 20 lines down already carries the right predicate, "refuse any spec that asks for a network guarantee" (`sbx.py:115-116`), so the two halves of the docstring now disagree. Reusing that wording over the new mechanism description fixes it, and it is worth adding that `block_network` defaults to `True`, since that default is what makes the two predicates different sets.
   
   One aside while this area is open: the `block_network` fact landed here and on the new page, but not in the canonical prose at `toolsets.rst:716-721`, which is now the least complete of the three near-identical copies.



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files

Review Comment:
   Worth naming the extra in this lead-in. `IcebergFormatHandler.register_data_source_format` raises `AirflowOptionalProviderFeatureException` unless `apache-airflow-providers-apache-iceberg` is installed (`common/sql/datafusion/format_handlers.py:106-114`), and it is an optional extra, `apache.iceberg` at `common/sql/pyproject.toml:100-101`. This block is the page's only Iceberg guidance, and line 330 already sets the page's precedent of flagging an extra inline. The `datafusion` extra the whole route needs (`common/sql/pyproject.toml:94-95`, stated in `toolsets/datafusion.py:97`) is the sibling gap worth covering in the same breath.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4045932227


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and
+  any function sqlglot does not recognize are rejected rather than allowed, so a
+  legitimate bespoke function needs naming in ``allowed_functions``. Statements
+  that modify data, ``COPY`` among them, are rejected either way while
+  ``allow_writes`` is ``False``.
+- It does not classify failures. Every exception from a tool becomes one
+  ``ModelRetry``, so a connection error and a typo in a column name are treated
+  the same way until the retry budget runs out and the task fails for Airflow to
+  retry.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV, Avro or Iceberg — and you want the agent to ask SQL
+questions of them without loading them anywhere first. Each
+``DataSourceConfig`` registers one table, and several can be registered so the
+agent can join across them.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+**What it cannot do**
+
+- The only backend that ships is not built for production. ``SbxSandboxBackend``
+  drives Docker Sandboxes, and its own documentation in this provider says to use
+  it for local development, calling a worker-driven run "off-label use". It wants
+  the ``sbx`` binary on the host, an authenticated Docker account, a one-time
+  ``sbx policy init``, and on Linux KVM or nested virtualization — which an
+  unprivileged container cannot provide. Anything beyond local development means
+  writing another backend behind
+  :class:`~airflow.providers.common.ai.sandbox.SandboxBackend`.
+- It does not contain the agent. Only what these tools do runs in the sandbox;
+  the agent loop, the model calls, and every other toolset on the same agent stay
+  in the worker with the worker's credentials. It contains model-written code, so
+  pairing it with a credential-bearing toolset on the same agent puts the
+  credential back within reach. :ref:`sandbox-boundaries` sets this out in full.
+- It cannot apply a per-sandbox network rule on the backend that ships today.
+  ``sbx`` governs egress through a host-level ``sbx policy``, and the backend says
+  so rather than letting a Dag author believe a per-sandbox restriction is in
+  force.
+- It does not guarantee reclamation. A failed teardown is logged as a warning
+  rather than raised, deliberately, so that a teardown blip cannot fail a
+  finished run. Nothing else picks up the slack: the backend that ships has no
+  server-side TTL to fall back on, so a worker killed outright leaves the microVM
+  and its workspace directory behind. They are named ``airflow-sandbox-*`` so an
+  operator can find and remove them — budget for that sweep.
+
+**A real example.** ``tests/system/common/ai/example_sandbox_toolset_sbx.py``
+in this provider is a runnable Dag against a real ``sbx`` host — the only
+toolset here with a system test. It is reachable from the System Tests entry in
+the sidebar.
+
+**Credentials and where it runs.** This is the one route that does not end at an

Review Comment:
   Dropped both absolutes — the sandbox route no longer calls itself "the one route that does not end at an Airflow connection", and the managed-agent bullet no longer says "unlike every other route here". Left the `MCPToolset` bullet as it stood: it says the credential need not come from a connection without claiming to be the only route where that holds.
   
   The managed-agent bullet now points at the Managed Agent Toolsets section for the intended shape — authentication routed through the provider's own hook — rather than reading as a flat contradiction of it.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4034783916


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and
+  any function sqlglot does not recognize are rejected rather than allowed, so a
+  legitimate bespoke function needs naming in ``allowed_functions``. Statements
+  that modify data, ``COPY`` among them, are rejected either way while
+  ``allow_writes`` is ``False``.
+- It does not classify failures. Every exception from a tool becomes one
+  ``ModelRetry``, so a connection error and a typo in a column name are treated
+  the same way until the retry budget runs out and the task fails for Airflow to
+  retry.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV, Avro or Iceberg — and you want the agent to ask SQL
+questions of them without loading them anywhere first. Each
+``DataSourceConfig`` registers one table, and several can be registered so the
+agent can join across them.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)

Review Comment:
   This line might not be really really needed. but I feel keeping it won't harm



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["kaxil (via GitHub)" <gi...@apache.org>]

kaxil merged PR #72936:
URL: https://github.com/apache/airflow/pull/72936


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4045531565


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose

Review Comment:
   Instead of adding a row for `code_mode=True`, I added a note above that entry, pointing at `code-mode` and saying what it does and does not.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["kaxil (via GitHub)" <gi...@apache.org>]

kaxil commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4042150812


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and
+  any function sqlglot does not recognize are rejected rather than allowed, so a
+  legitimate bespoke function needs naming in ``allowed_functions``. Statements
+  that modify data, ``COPY`` among them, are rejected either way while
+  ``allow_writes`` is ``False``.
+- It does not classify failures. Every exception from a tool becomes one
+  ``ModelRetry``, so a connection error and a typo in a column name are treated
+  the same way until the retry budget runs out and the task fails for Airflow to
+  retry.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV, Avro or Iceberg — and you want the agent to ask SQL
+questions of them without loading them anywhere first. Each
+``DataSourceConfig`` registers one table, and several can be registered so the
+agent can join across them.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+**What it cannot do**
+
+- The only backend that ships is not built for production. ``SbxSandboxBackend``
+  drives Docker Sandboxes, and its own documentation in this provider says to use
+  it for local development, calling a worker-driven run "off-label use". It wants
+  the ``sbx`` binary on the host, an authenticated Docker account, a one-time
+  ``sbx policy init``, and on Linux KVM or nested virtualization — which an
+  unprivileged container cannot provide. Anything beyond local development means
+  writing another backend behind
+  :class:`~airflow.providers.common.ai.sandbox.SandboxBackend`.
+- It does not contain the agent. Only what these tools do runs in the sandbox;
+  the agent loop, the model calls, and every other toolset on the same agent stay
+  in the worker with the worker's credentials. It contains model-written code, so
+  pairing it with a credential-bearing toolset on the same agent puts the
+  credential back within reach. :ref:`sandbox-boundaries` sets this out in full.
+- It cannot apply a per-sandbox network rule on the backend that ships today.
+  ``sbx`` governs egress through a host-level ``sbx policy``, and the backend says
+  so rather than letting a Dag author believe a per-sandbox restriction is in
+  force.
+- It does not guarantee reclamation. A failed teardown is logged as a warning
+  rather than raised, deliberately, so that a teardown blip cannot fail a
+  finished run. Nothing else picks up the slack: the backend that ships has no
+  server-side TTL to fall back on, so a worker killed outright leaves the microVM
+  and its workspace directory behind. They are named ``airflow-sandbox-*`` so an
+  operator can find and remove them — budget for that sweep.
+
+**A real example.** ``tests/system/common/ai/example_sandbox_toolset_sbx.py``
+in this provider is a runnable Dag against a real ``sbx`` host — the only
+toolset here with a system test. It is reachable from the System Tests entry in
+the sidebar.
+
+**Credentials and where it runs.** This is the one route that does not end at an

Review Comment:
   Three routes on this page each claim to be the unique exception here: this line, the `BaseManagedAgentToolset` bullet at 405 ("unlike every other route here"), and the `MCPToolset` bullet at 250 ("it cannot guarantee the credential came from a connection"). Only one of the three can hold. The credentials table below gets all three right, so it's the prose absolutes that need fixing. On the managed-agent one, `toolsets.rst:1208` states the opposite intent: "Each provider package ships its own subclass, so credentials keep flowing through that provider's existing hook."



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows

Review Comment:
   Iceberg doesn't really sit under "files on an object store". `DataSourceConfig` treats it as catalog-managed: `uri` and `storage_type` go unused, and `__post_init__` raises `ValueError("Database name must be provided for table providers")` when `db_name` is absent (`common/sql/config.py:90-93`). Someone who routes here for Iceberg and then copies the page's only DataFusion example, which is parquet-shaped with no `db_name`, hits that error. `common/sql/.../example_dags/example_analytics.py:33` has the Iceberg-shaped config if a second example is worth it.



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and

Review Comment:
   The sentence above says `SHOW` passes, and this fail-closed list doesn't mention it, so a reader turning on `allowed_tables` won't expect `SHOW` to break. It does: `collect_table_references` flags `exp.Show` as unverifiable (`utils/sql_validation.py:307`), as it does an inline comment anywhere in the query (line 360). The list names three of the ten constructs the walk refuses, and since `allowed-tables-enforcement` is already linked a few lines up, deferring the enumeration to it may read better than naming three. Separately, "every exception from a tool becomes one `ModelRetry`" below holds for three of the four tools but not `check_query`, which catches everything itself and returns `{"valid": false, "error": ...}` (`sql.py:483-494`).



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose

Review Comment:
   Should `code_mode=True` be a row here? It's the other answer to "code the model wrote", `operators/agent.rst:401` documents it, and `toolsets.rst:643-666` already tables the two against each other ("It is not a sandbox for your data; it restricts generated glue code to the tools you registered"). It matters more on this page than on that one, since this row sends a reader to a toolset whose only backend you go on to describe as off-label for workers.



##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and
+  any function sqlglot does not recognize are rejected rather than allowed, so a
+  legitimate bespoke function needs naming in ``allowed_functions``. Statements
+  that modify data, ``COPY`` among them, are rejected either way while
+  ``allow_writes`` is ``False``.
+- It does not classify failures. Every exception from a tool becomes one
+  ``ModelRetry``, so a connection error and a typo in a column name are treated
+  the same way until the retry budget runs out and the task fails for Airflow to
+  retry.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV, Avro or Iceberg — and you want the agent to ask SQL
+questions of them without loading them anywhere first. Each
+``DataSourceConfig`` registers one table, and several can be registered so the
+agent can join across them.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+**What it cannot do**
+
+- The only backend that ships is not built for production. ``SbxSandboxBackend``
+  drives Docker Sandboxes, and its own documentation in this provider says to use
+  it for local development, calling a worker-driven run "off-label use". It wants
+  the ``sbx`` binary on the host, an authenticated Docker account, a one-time
+  ``sbx policy init``, and on Linux KVM or nested virtualization — which an
+  unprivileged container cannot provide. Anything beyond local development means
+  writing another backend behind
+  :class:`~airflow.providers.common.ai.sandbox.SandboxBackend`.
+- It does not contain the agent. Only what these tools do runs in the sandbox;
+  the agent loop, the model calls, and every other toolset on the same agent stay
+  in the worker with the worker's credentials. It contains model-written code, so
+  pairing it with a credential-bearing toolset on the same agent puts the
+  credential back within reach. :ref:`sandbox-boundaries` sets this out in full.
+- It cannot apply a per-sandbox network rule on the backend that ships today.

Review Comment:
   `SbxSandboxBackend.create` does apply a per-sandbox rule: when the spec names `allow_egress_to` it runs `sbx policy allow network --sandbox NAME <hosts>` (`sandbox/sbx.py:310-322`). What it can't do is narrow an open host policy, which is why `_check_spec` refuses the spec unless `host_network_policy="deny-all"` was declared (`sbx.py:165-173`). `block_network` is the field with no per-sandbox enforcement, not `allow_egress_to`, and `toolsets.rst:715` already draws that line. As written this bullet steers a reader away from something the backend can do. The phrasing traces back to the class docstring at `sbx.py:83-88`, which contradicts its own `create`, so that's worth correcting alongside.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4045914410


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,533 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro, Iceberg — rather than rows
+       in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is unset,
+  so out of the box the agent reaches every table the connection can see.
+  ``DESCRIBE`` and ``SHOW`` pass as well, on dialects that parse them, because
+  read-only metadata statements are allowed deliberately. Set ``allowed_tables``
+  and the walk turns fail-closed: dynamic SQL, the ``TABLE <name>`` shorthand and
+  any function sqlglot does not recognize are rejected rather than allowed, so a
+  legitimate bespoke function needs naming in ``allowed_functions``. Statements
+  that modify data, ``COPY`` among them, are rejected either way while
+  ``allow_writes`` is ``False``.
+- It does not classify failures. Every exception from a tool becomes one
+  ``ModelRetry``, so a connection error and a typo in a column name are treated
+  the same way until the retry budget runs out and the task fails for Airflow to
+  retry.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV, Avro or Iceberg — and you want the agent to ask SQL
+questions of them without loading them anywhere first. Each
+``DataSourceConfig`` registers one table, and several can be registered so the
+agent can join across them.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.
+  :class:`~airflow.providers.common.ai.toolsets.mcp.MCPToolset` forwards
+  ``get_tools`` and ``call_tool`` straight to the underlying server, so whatever
+  the server exposes, the agent gets. ``tool_prefix`` renames; it does not
+  filter. The defense-layer table is explicit that a server can expose shell,
+  filesystem or network access.
+- It cannot guarantee the credential came from a connection. ``mcp_conn_id`` is
+  the default path, but ``token_provider`` and ``env_provider`` are your own
+  callables and are free to read an environment variable, a file, or an entirely
+  different secret store. That is the point of them — it also means the
+  connection is no longer the whole story for anyone auditing the Dag.
+- ``stdio`` transport is not isolation. It starts a child process on the worker
+  host. It is not a sandbox, and when no environment is supplied the child
+  inherits a small allowlist of variables rather than the full parent
+  environment — so it is both less contained and less predictable than it looks.
+
+**A real example.** ``example_mcp.py`` drives setup from a connection:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_connection]
+    :end-before: [END howto_toolset_mcp_connection]
+
+and puts several servers on one agent, prefixed so their tool names stay apart:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_mcp.py
+    :language: python
+    :start-after: [START howto_toolset_mcp_multiple]
+    :end-before: [END howto_toolset_mcp_multiple]
+
+No MCP server for object storage ships with this provider. If one exists for
+your target, it is a third route alongside the two above — and the two questions
+from the start of this page settle it. Its tool list is the server's rather than
+yours, and its token comes from wherever ``mcp_conn_id`` or your own callable
+says. So where a hook already reaches the same target, the hook wins; the server
+wins where it covers work the hook does not expose.
+
+**Credentials and where it runs.** ``mcp_conn_id`` supplies host, credentials and
+transport, unless a provider callable overrides that. ``http`` and ``sse`` reach
+a remote server; ``stdio`` runs a child process on the worker host.
+
+``AgentSkillsToolset``
+----------------------
+
+**Choose it when** what the agent is missing is procedural knowledge rather than
+an endpoint — how this team writes a report, which checks run before a release,
+what the house conventions are. A skill is a directory of instructions and
+optional scripts, and
+:class:`~airflow.providers.common.ai.toolsets.skills.AgentSkillsToolset` makes it
+discoverable. See :ref:`agent-skills` for the layout.
+
+**What it cannot do**
+
+- ``exclude_resources`` does not hide a file from the skill's own scripts. It
+  keeps matches out of resource discovery and out of ``read_skill_resource``,
+  and the parameter's documentation says plainly that it does not stop
+  ``run_skill_script`` from reading them off disk. For genuinely sensitive files,
+  pair it with ``exclude_tools={"run_skill_script"}``. (The parameter needs
+  ``pydantic-ai-skills>=1.2.0``, which the ``skills`` extra already pins.)
+- It does not move script execution anywhere safer. The toolset's own wording for
+  ``exclude_tools`` calls ``run_skill_script`` "on-worker script execution", and
+  nothing in this toolset routes those scripts into a sandbox — so unless you
+  exclude the tool, a skill's scripts run in the worker process with the worker's
+  reach. If that is not acceptable, exclude the tool or put the work behind
+  ``SandboxToolset`` instead.
+- It re-fetches a Git source on every run. ``GitSkills`` is resolved and
+  shallow-cloned on the worker when the run starts, and the checkout is deleted
+  when it ends; nothing is kept between runs, so a large or slow repository pays
+  that clone once per run. A local directory is read in place and
+  costs nothing.
+
+**A real example.** ``example_agent_skills.py`` loads skills from a local
+directory:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_agent_skills.py
+    :language: python
+    :start-after: [START howto_operator_agent_skills_local]
+    :end-before: [END howto_operator_agent_skills_local]
+
+The two skills it ships, ``aip-tracker`` and ``sql-reporting``, are procedural by
+nature: neither adds an endpoint the agent could not already reach. That is the
+signal you are on the right route.
+
+**Credentials and where it runs.** A local directory needs no credential. A
+private repository goes through
+:class:`~airflow.providers.common.ai.skills.GitSkills` and its ``conn_id``,
+resolved by the Git provider's ``GitHook``; plain ``http://`` is refused when a
+``conn_id`` is set, so a credential is never sent in the clear. Cloning, reading
+and any script execution happen on the worker.
+
+``SandboxToolset``
+------------------
+
+**Choose it when** the work is running code the model wrote, rather than calling
+a tool you picked in advance — exploratory analysis, installing a package for one
+task, producing a file. Every other route on this page answers "call this thing";
+this one answers "here is somewhere to work".
+
+**What it cannot do**
+
+- The only backend that ships is not built for production. ``SbxSandboxBackend``
+  drives Docker Sandboxes, and its own documentation in this provider says to use
+  it for local development, calling a worker-driven run "off-label use". It wants
+  the ``sbx`` binary on the host, an authenticated Docker account, a one-time
+  ``sbx policy init``, and on Linux KVM or nested virtualization — which an
+  unprivileged container cannot provide. Anything beyond local development means
+  writing another backend behind
+  :class:`~airflow.providers.common.ai.sandbox.SandboxBackend`.
+- It does not contain the agent. Only what these tools do runs in the sandbox;
+  the agent loop, the model calls, and every other toolset on the same agent stay
+  in the worker with the worker's credentials. It contains model-written code, so
+  pairing it with a credential-bearing toolset on the same agent puts the
+  credential back within reach. :ref:`sandbox-boundaries` sets this out in full.
+- It cannot apply a per-sandbox network rule on the backend that ships today.

Review Comment:
   `allow_egress_to` is applied as a per-sandbox rule, but only on top of a `deny-all` host policy, since a local rule can narrow egress and never widen it
   `block_network` is the one with no per-sandbox enforcement. `SbxSandboxBackend`'s class docstring says the same thing now instead of contradicting its own `create`.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add a decision guide for choosing between common.ai toolsets [airflow]

["Lee-W (via GitHub)" <gi...@apache.org>]

Lee-W commented on code in PR #72936:
URL: https://github.com/apache/airflow/pull/72936#discussion_r4052573109


##########
providers/common/ai/docs/choosing_a_toolset.rst:
##########
@@ -0,0 +1,574 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License.  You may obtain a copy of the License at
+
+ ..   http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied.  See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+.. _howto/choosing-a-toolset:
+
+Choosing a Toolset
+==================
+
+:doc:`toolsets` documents how to configure each toolset. This page answers the
+question that comes before that one: you have a system you want an agent to
+reach, so which route do you take, and what does each route give up?
+
+Read the table below by what you already have, not by what a toolset is called.
+When two routes both work, the deciding factor is rarely what each one can do —
+it is what each one cannot do, and every route has a short list.
+
+More than one row can be true at once, and the rows are not exclusive: one agent
+can carry several toolsets. Two questions break the ties. *Whose credential is
+it?* — prefer the route whose credential is an Airflow connection somebody on
+your side already reviewed. *Whose tool list is it?* — prefer the route whose
+exposed surface you chose rather than inherited. The pair that most often
+overlaps is an Airflow hook and a vendor MCP server reaching the same target;
+both questions point at the hook, because its credential is the connection and
+``allowed_methods`` is a list you write. Reach for the server when its tools
+cover work the hook does not expose, or when the alternative is re-wrapping that
+API by hand.
+
+Those two questions do not separate ``HookToolset`` from ``SQLToolset`` when the
+target is a DBAPI database, because both answer them the same way. A third one
+does: *is the work a fixed operation or an open-ended question?* A named method
+you can enumerate in advance is a hook. A question the agent has to express as
+SQL is a query, and ``SQLToolset`` answers it with schema discovery, bounded
+results and an ``allowed_tables`` walk you can switch on, none of which
+``HookToolset`` has an equivalent of.
+
+Start with what you have
+------------------------
+
+.. list-table::
+   :widths: 50 50
+   :header-rows: 1
+
+   * - What you have
+     - Route
+   * - A target that already has an Airflow connection, and a hook method that
+       already does the thing
+     - ``HookToolset``
+   * - A question that is a query, against a DBAPI database
+     - ``SQLToolset``
+   * - Files on an object store — Parquet, CSV, Avro — or a catalog-managed
+       table format such as Iceberg, rather than rows in a database
+     - ``DataFusionToolset``
+   * - A vendor that already ships a server built for agents, whose tools you
+       would otherwise re-wrap by hand
+     - ``MCPToolset``
+   * - Procedural knowledge — how to carry out a task — rather than an endpoint
+       to call
+     - ``AgentSkillsToolset``
+   * - Work that means running code the model wrote, not calling a tool you chose
+     - ``SandboxToolset``
+   * - Reasoning that should happen on the vendor's own infrastructure
+     - A subclass of ``BaseManagedAgentToolset`` that you write
+
+The rest of this page takes those seven in turn. Each entry gives the case for
+choosing it, what it cannot do, an example that exists in this repository, and
+where its credentials and its work come from.
+
+``HookToolset``
+---------------
+
+**Choose it when** the target already has an Airflow connection and a hook, and
+what you want the agent to do is already a method on that hook. This is the
+cheapest route — no new server, no new credential, no new query dialect — and
+the only one that reaches any provider hook with synchronous methods without
+anyone writing an adapter first. :class:`~airflow.providers.common.ai.toolsets.hook.HookToolset` is a
+reflection-based adapter, so the work is choosing the method list.
+
+**What it cannot do**
+
+- It allow-lists method *names*, not arguments. Once ``read_key`` is exposed,
+  the agent picks the key; the :ref:`defense-layer table <toolset-defense-layers>`
+  states this outright. Choose methods whose worst case you accept, not methods
+  you intend to constrain later.
+- Its calls act as barriers. The tools are registered with ``sequential=True``
+  because hook methods perform synchronous I/O, so a slow call holds up every
+  other tool the model emitted in that step, not only this toolset's. This is
+  not specific to ``HookToolset`` — see :ref:`toolset-call-barriers`.
+- 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.
+- Its ``call_tool`` calls the method and serializes what comes back. The code
+  contains no path that awaits a coroutine result, and none that checks for one,
+  so an ``async def`` hook method is not a case this adapter is written to
+  handle. Treat synchronous methods as the supported set.
+
+**A real example.** The read-only S3 pair from the ``HookToolset`` guidance in
+:doc:`toolsets`:
+
+.. code-block:: python
+
+    HookToolset(
+        s3_hook,
+        allowed_methods=["list_keys", "read_key"],
+        tool_name_prefix="s3_",
+    )
+
+**Credentials and where it runs.** The hook instance is yours, so the credential
+is whatever connection that hook resolves — the toolset never looks one up
+itself. Calls run in the Airflow worker process.
+
+``SQLToolset``
+--------------
+
+**Choose it when** the question is a query and the data is in a DBAPI database.
+:class:`~airflow.providers.common.ai.toolsets.sql.SQLToolset` gives the agent
+four tools — list tables, get schema, query, check query. Set ``allowed_tables``
+and that allow-list is enforced by parsing the SQL rather than by matching
+strings; see :ref:`allowed-tables-enforcement` for how the walk handles CTEs,
+subqueries and joins.
+
+**What it cannot do**
+
+- ``allowed_tables`` is an application-level guardrail, not a replacement for
+  database permissions. Its own docstring says so, and names the residual gap:
+  an engine or query the parser reads differently. Point ``db_conn_id`` at a
+  least-privilege role whose grants match the allow-list.
+- It cannot bound the fetch for every driver. Hooks that hand their handler
+  something other than a DBAPI cursor — ``ExasolHook`` and its pyexasol
+  statement, for instance — fall back to a full fetch. The payload handed to the
+  model is still bounded; the transfer is not. See :ref:`bounded-query-results`.
+- Its parser-level closure is opt-in, not the default. ``allowed_tables``
+  defaults to ``None`` and the table walk returns immediately while it is
+  unset, so out of the box the agent reaches every table the connection can
+  see. ``DESCRIBE`` and ``SHOW`` both pass, on dialects that parse them,
+  while ``allowed_tables`` stays unset. Set ``allowed_tables`` and the walk
+  turns fail-closed for ``SHOW`` — but not for ``DESCRIBE``: it instead
+  becomes an ordinary table reference, allowed only when the table it names
+  is on the list. See :ref:`allowed-tables-enforcement` for what else the
+  walk rejects once ``allowed_tables`` is active. Statements that modify
+  data, ``COPY`` among them, are rejected either way while ``allow_writes``
+  is ``False``.
+- It does not classify failures. A connection error or a typo in a column
+  name reaching ``list_tables``, ``get_schema`` or ``query`` becomes one
+  ``ModelRetry``, so the two are treated the same way until the retry budget
+  runs out and the task fails for Airflow to retry. Two paths do not raise:
+  ``check_query`` catches its own errors and reports them back as a normal
+  ``{"valid": false, ...}`` result, and ``get_schema`` returns a normal
+  ``{"error": ...}`` result instead of raising when the requested table is
+  outside ``allowed_tables`` — other ``get_schema`` failures still raise and
+  still become a ``ModelRetry``.
+
+**A real example.** ``example_pydantic_ai_hook.py`` builds an agent around
+``SQLToolset`` inside a plain ``@task`` function, with no operator involved:
+
+.. exampleinclude:: /../../ai/src/airflow/providers/common/ai/example_dags/example_pydantic_ai_hook.py
+    :language: python
+    :start-after: [START howto_task_with_toolsets]
+    :end-before: [END howto_task_with_toolsets]
+
+**Credentials and where it runs.** ``db_conn_id`` is resolved through
+``BaseHook.get_connection``, and the connection must supply a ``DbApiHook``.
+Queries run from the Airflow worker process against the database. Its tool
+calls act as barriers, as they do for the other routes that build their own
+tools; see :ref:`toolset-call-barriers`.
+
+``DataFusionToolset``
+---------------------
+
+**Choose it when** the data is files on an object store rather than rows in a
+database — Parquet, CSV or Avro — or a table in a catalog such as Iceberg, and
+you want the agent to ask SQL questions of them without loading them anywhere
+first. Each ``DataSourceConfig`` registers one table, and several can be
+registered so the agent can join across them. The two shapes take different
+fields: an object-store format needs a ``uri``, while a catalog format like
+Iceberg is looked up by ``db_name`` instead, and ``DataSourceConfig`` raises
+``ValueError`` at construction if a catalog format is missing one.
+
+**What it cannot do**
+
+- It has no table allow-list. ``allow_writes=False`` is the only guard, and it
+  blocks non-SELECT statements, not reach: the defense-layer table records that
+  this toolset "does not prevent the agent from reading any registered data
+  source". The registration list is therefore the whole boundary — register
+  exactly what the agent may read.
+- It bounds the payload, not the scan. DataFusion has already materialized the
+  full result before ``max_rows`` and ``max_result_bytes`` apply, so those limits
+  protect the model's context, not the cost of the query.
+- It cannot tell failure kinds apart precisely. The DataFusion Python bindings
+  expose no native exception types, so the retry decision is made by matching the
+  error message against regular expressions — which a wording change upstream can
+  quietly defeat.
+
+**A real example.** The same bucket as the ``HookToolset`` entry above, reached
+the other way. Rather than exposing ``list_keys`` and ``read_key`` and leaving
+the agent to reassemble files, this registers the prefix as a table and lets it
+write SQL:
+
+.. code-block:: python
+
+    from airflow.providers.common.ai.toolsets.datafusion import DataFusionToolset
+    from airflow.providers.common.sql.config import DataSourceConfig
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="aws_default",
+                table_name="sales",
+                uri="s3://my-bucket/data/sales/",
+                format="parquet",
+            ),
+        ],
+        max_rows=100,
+    )
+
+Which of the two fits depends on the question. "Read me this object" is a hook
+method. "What were last quarter's returns by region" is a query, and expressing
+it through ``list_keys`` and ``read_key`` means the model does the aggregation in
+its context window instead of the engine doing it.
+
+An Iceberg table is registered differently. There is no ``uri`` to read files
+from; the catalog resolves the table by name, so the config carries a
+``db_name`` instead, following the same ``DataSourceConfig`` shape that
+``example_analytics.py`` in the ``common.sql`` provider uses:
+
+.. code-block:: python
+
+    toolset = DataFusionToolset(
+        datasource_configs=[
+            DataSourceConfig(
+                conn_id="iceberg_default",
+                table_name="users_data",
+                db_name="demo",
+                format="iceberg",
+            ),
+        ],
+        max_rows=100,
+    )
+
+**Credentials and where it runs.** Each ``DataSourceConfig`` carries its own
+``conn_id``, so object-store access is an Airflow connection. DataFusion is an
+embedded engine: the query runs inside the worker process, not on a remote
+cluster. Its tool calls act as barriers, as they do for the other routes that
+build their own tools; see :ref:`toolset-call-barriers`.
+
+``MCPToolset``
+--------------
+
+**Choose it when** someone already publishes a server built for agents that
+covers your target. You inherit a tool surface that was designed to be called by
+a model — retry semantics and error wording are decided upstream, and a
+destructive tool can simply be absent — instead of maintaining a per-API wrapper
+yourself.
+
+**What it cannot do**
+
+- It has no tool-level allow-list at all.

Review Comment:
   Change it to that the allow-list is client-side and opt-in, not server-side and required — and names `.filtered()` as inherited from the `AbstractToolset` base every toolset in this provider extends. 
   
   The filtering narrows what the agent is offered without revoking or authorizing anything on the server, and nothing requires you to set it, unlike `allowed_methods` on `HookToolset`, which rejects an empty list.
   
   The `tool_prefix` sentence is gone.
   
   I checked `filtered` at the `pydantic-ai-slim` floor rather than the installed version, so the bullet carries no version caveat.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org