Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/app/endpoints/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
mcp_headers_dependency,
)
from utils.mcp_oauth_probe import check_mcp_auth
from utils.pydantic_ai import get_agent_capability_tools
from utils.tool_formatter import format_tools_list

logger = get_logger(__name__)
Expand Down Expand Up @@ -238,11 +239,25 @@ async def tools_endpoint_handler( # pylint: disable=too-many-locals,too-many-st
server_source,
)

existing_tool_ids = {
tool.get("identifier") for tool in consolidated_tools if tool.get("identifier")
}
capability_tools = get_agent_capability_tools(configuration.skills)
for tool_dict in capability_tools:
identifier = tool_dict.get("identifier")
if identifier and identifier not in existing_tool_ids:
consolidated_tools.append(tool_dict)
existing_tool_ids.add(identifier)
Comment on lines +242 to +250

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Deduplicate capability tools by source, not by name alone.

This block only keys on identifier, but /tools already carries provider_id and toolgroup_id because same-named tools can exist in different sources. With the current logic, an MCP or builtin tool named list_skills/load_skill will silently suppress the capability tool, which defeats the discoverability this PR is adding.

Suggested fix
-    existing_tool_ids = {
-        tool.get("identifier") for tool in consolidated_tools if tool.get("identifier")
-    }
+    existing_tool_keys = {
+        (
+            tool.get("identifier"),
+            tool.get("provider_id"),
+            tool.get("toolgroup_id"),
+        )
+        for tool in consolidated_tools
+        if tool.get("identifier")
+    }
     capability_tools = get_agent_capability_tools(configuration.skills)
     for tool_dict in capability_tools:
-        identifier = tool_dict.get("identifier")
-        if identifier and identifier not in existing_tool_ids:
+        key = (
+            tool_dict.get("identifier"),
+            tool_dict.get("provider_id"),
+            tool_dict.get("toolgroup_id"),
+        )
+        if key[0] and key not in existing_tool_keys:
             consolidated_tools.append(tool_dict)
-            existing_tool_ids.add(identifier)
+            existing_tool_keys.add(key)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
existing_tool_ids = {
tool.get("identifier") for tool in consolidated_tools if tool.get("identifier")
}
capability_tools = get_agent_capability_tools(configuration.skills)
for tool_dict in capability_tools:
identifier = tool_dict.get("identifier")
if identifier and identifier not in existing_tool_ids:
consolidated_tools.append(tool_dict)
existing_tool_ids.add(identifier)
existing_tool_keys = {
(
tool.get("identifier"),
tool.get("provider_id"),
tool.get("toolgroup_id"),
)
for tool in consolidated_tools
if tool.get("identifier")
}
capability_tools = get_agent_capability_tools(configuration.skills)
for tool_dict in capability_tools:
key = (
tool_dict.get("identifier"),
tool_dict.get("provider_id"),
tool_dict.get("toolgroup_id"),
)
if key[0] and key not in existing_tool_keys:
consolidated_tools.append(tool_dict)
existing_tool_keys.add(key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/endpoints/tools.py` around lines 242 - 250, The deduplication in the
consolidated_tools merge only uses identifier, so capability tools can be
incorrectly skipped when a same-named tool exists from a different source.
Update the merge logic in the tools endpoint to deduplicate by the full source
identity, using provider_id and toolgroup_id alongside identifier for capability
tools added via get_agent_capability_tools(configuration.skills). Keep existing
behavior for unique tools, but ensure /tools can surface same-named tools from
different providers or groups without suppressing discoverability.


builtin_tool_count = len(
[t for t in consolidated_tools if t.get("server_source") == "builtin"]
)
mcp_tool_count = len(consolidated_tools) - builtin_tool_count
logger.info(
"Retrieved total of %d tools (%d from built-in toolgroups, %d from MCP servers)",
len(consolidated_tools),
len([t for t in consolidated_tools if t.get("server_source") == "builtin"]),
len([t for t in consolidated_tools if t.get("server_source") != "builtin"]),
builtin_tool_count,
mcp_tool_count,
)

# Format tools with structured description parsing
Expand Down
92 changes: 92 additions & 0 deletions src/utils/pydantic_ai.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import re
from typing import Any, Final, Optional, cast

from llama_stack.core.library_client import AsyncLlamaStackAsLibraryClient
Expand All @@ -18,6 +19,11 @@
LlamaStackResponsesModel,
)

_AGENT_SKILLS_PROVIDER_ID: Final[str] = "agent-skills"
_AGENT_SKILLS_TOOLGROUP_ID: Final[str] = "builtin::agent-skills"
_BUILTIN_CAPABILITY_SERVER_SOURCE: Final[str] = "builtin"
_CAPABILITY_TOOL_TYPE: Final[str] = "tool"

_LLS_RESPONSES_EXTRA_FIELDS: Final[frozenset[str]] = frozenset(
{
"conversation",
Expand Down Expand Up @@ -96,6 +102,92 @@ def _skills_capability(
)


def _json_schema_to_parameters(
schema: Optional[dict[str, Any]],
) -> list[dict[str, Any]]:
"""Convert a JSON Schema object to the flat parameter list used by ``/tools``."""
if not schema or "properties" not in schema:
return []

required_params = set(schema.get("required", []))
parameters: list[dict[str, Any]] = []
for name, prop in schema["properties"].items():
parameter_type = prop.get("type")
if parameter_type is None and "anyOf" in prop:
for option in prop["anyOf"]:
if isinstance(option, dict) and option.get("type") not in (
None,
"null",
):
parameter_type = option["type"]
break
parameters.append(
{
"name": name,
"description": prop.get("description", ""),
"parameter_type": parameter_type or "string",
"required": name in required_params,
"default": prop.get("default"),
}
)
return parameters


def _capability_tool_description(description: str) -> str:
"""Extract a user-facing description from pydantic-ai tool docstrings."""
if match := re.search(r"<summary>(.*?)</summary>", description, re.DOTALL):
return match.group(1).strip()
return description.strip()


def _capability_tools_from_toolset(toolset: Any) -> list[dict[str, Any]]:
"""Serialize tools registered on a pydantic-ai capability toolset."""
raw_tools = getattr(toolset, "tools", None)
Comment on lines +105 to +145

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Expand the new helper docstrings to the repo format.

These three helpers only have one-line docstrings right now. In this repo, functions under src/**/*.py are expected to carry full Parameters: / Returns: / Raises: sections, so these new utilities are out of step with the local documentation contract.

As per coding guidelines, src/**/*.py functions must “include descriptive docstrings” and “follow Google Python docstring conventions with required sections: Parameters, Returns, Raises”; based on learnings, this repo uses Parameters: rather than Args:.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/pydantic_ai.py` around lines 105 - 145, The new helper docstrings
in _json_schema_to_parameters, _capability_tool_description, and
_capability_tools_from_toolset are too minimal for this repo’s docstring
standard. Update each function’s docstring to the local Google-style format with
clear descriptive text plus Parameters:, Returns:, and Raises: sections where
applicable, keeping the wording aligned with other functions under src/**/*.py.

Sources: Coding guidelines, Learnings

if not raw_tools:
return []

tool_dicts: list[dict[str, Any]] = []
for tool in raw_tools.values():
tool_dicts.append(
{
"identifier": tool.name,
"description": _capability_tool_description(tool.description or ""),
"parameters": _json_schema_to_parameters(
tool.function_schema.json_schema
),
"provider_id": _AGENT_SKILLS_PROVIDER_ID,
"toolgroup_id": _AGENT_SKILLS_TOOLGROUP_ID,
"server_source": _BUILTIN_CAPABILITY_SERVER_SOURCE,
"type": _CAPABILITY_TOOL_TYPE,
}
)
return tool_dicts


def get_agent_capability_tools(
skills: Optional[SkillsConfiguration],
) -> list[dict[str, Any]]:
"""Return tool metadata for pydantic-ai capabilities configured for LCS agents.

Parameters:
skills: Agent skills configuration from LCS, or None when skills are disabled.

Returns:
Tool dictionaries compatible with the ``/tools`` endpoint response format.
"""
Comment on lines +167 to +177

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Normalize touched docstrings to the required section format.

get_agent_capability_tools and build_agent still omit the required Raises: section, and _agent_capabilities uses Args: instead of the repo’s Parameters: header.

Suggested docstring cleanup
 def get_agent_capability_tools(
     skills: Optional[SkillsConfiguration],
 ) -> list[dict[str, Any]]:
@@
     Returns:
         Tool dictionaries compatible with the ``/tools`` endpoint response format.
+
+    Raises:
+        None.
     """
@@
 def _agent_capabilities(
@@
-    Args:
+    Parameters:
         skills: Agent skills configuration from LCS, or None when skills are disabled.
         no_tools: When True, omit capabilities that expose a toolset via ``get_toolset()``.
 
     Returns:
         Configured capabilities, or None when no capabilities are enabled.
+
+    Raises:
+        None.
     """
@@
     Returns:
         ``Agent`` configured for ``await agent.run(...)`` (or streaming) against the same
         stack configuration as ``client.responses.create(**responses_params.model_dump())``.
+
+    Raises:
+        None.
     """

As per coding guidelines, src/**/*.py functions must include descriptive docstrings with Parameters, Returns, and Raises; based on learnings, use Parameters: rather than Args:.

Also applies to: 195-203, 232-241

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/pydantic_ai.py` around lines 167 - 177, Normalize the touched
docstrings in get_agent_capability_tools, build_agent, and _agent_capabilities
to the repo’s required section format. Add a descriptive Raises: section to
get_agent_capability_tools and build_agent, and replace any Args: header in
_agent_capabilities with Parameters: so all three docstrings consistently use
Parameters:, Returns:, and Raises: where applicable.

Sources: Coding guidelines, Learnings

capabilities = _agent_capabilities(skills) or []

tools: list[dict[str, Any]] = []
for capability in capabilities:
if not isinstance(capability, AbstractCapability):
continue
toolset = capability.get_toolset()
if toolset is None:
continue
tools.extend(_capability_tools_from_toolset(toolset))
return tools


def _agent_capabilities(
skills: Optional[SkillsConfiguration],
no_tools: bool = False,
Expand Down
42 changes: 42 additions & 0 deletions tests/unit/app/endpoints/test_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
LlamaStackConfiguration,
ModelContextProtocolServer,
ServiceConfiguration,
SkillsConfiguration,
TLSConfiguration,
UserDataCollection,
)
Expand Down Expand Up @@ -1135,3 +1136,44 @@ async def test_tools_endpoint_empty_legacy_fields_overridden(
assert tool["parameters"][0]["name"] == "query"
assert tool["parameters"][0]["parameter_type"] == "string"
assert tool["parameters"][0]["required"] is True


@pytest.mark.asyncio
async def test_tools_endpoint_includes_agent_capability_tools(
mocker: MockerFixture,
mock_configuration: Configuration, # pylint: disable=redefined-outer-name
mock_skills_configuration: SkillsConfiguration,
) -> None:
"""Test that configured pydantic-ai capabilities appear in /tools output."""
config_with_skills = mock_configuration.model_copy(
update={"skills": mock_skills_configuration}
)
app_config = AppConfig()
app_config._configuration = config_with_skills
mocker.patch("app.endpoints.tools.configuration", app_config)
mocker.patch("app.endpoints.tools.authorize", lambda _: lambda func: func)

mock_client_holder = mocker.patch("app.endpoints.tools.AsyncLlamaStackClientHolder")
mock_client = mocker.AsyncMock()
mock_client_holder.return_value.get_client.return_value = mock_client
mock_client.toolgroups.list.return_value = []

mock_request = mocker.Mock()
mock_auth = MOCK_AUTH

response = await tools.tools_endpoint_handler.__wrapped__( # pyright: ignore
mock_request, mock_auth, {}
)

tool_ids = [tool["identifier"] for tool in response.tools]
assert "list_skills" in tool_ids
assert "load_skill" in tool_ids
assert "read_skill_resource" in tool_ids
assert "run_skill_script" in tool_ids

list_skills = next(
tool for tool in response.tools if tool["identifier"] == "list_skills"
)
assert list_skills["provider_id"] == "agent-skills"
assert list_skills["toolgroup_id"] == "builtin::agent-skills"
assert list_skills["server_source"] == "builtin"
Comment on lines +1156 to +1179

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a duplicate-identifier case for the new merge logic.

This test proves inclusion, but not the identifier-based deduplication called out in the PR. With toolgroups.list mocked to [], the branch that skips capability tools already present from builtin/MCP sources stays untested. Add one fixture toolgroup entry with an identifier like list_skills and assert the response only contains it once.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/app/endpoints/test_tools.py` around lines 1156 - 1179, The current
test only verifies that capability tools are included, but it does not cover the
identifier-based deduplication in tools.tools_endpoint_handler. Update the
mocked toolgroups.list setup in the same test to return one toolgroup entry
whose tool has an identifier matching a builtin capability tool such as
list_skills, then assert the final response.tools contains that identifier only
once. Keep the existing checks for tools_endpoint_handler.__wrapped__,
AsyncLlamaStackClientHolder, and tool_ids, but add the duplicate-case assertion
to validate the merge logic.

44 changes: 44 additions & 0 deletions tests/unit/utils/test_pydantic_ai.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
_model_settings_from_responses_params,
_skills_capability,
build_agent,
get_agent_capability_tools,
llama_stack_provider_from_client,
)

Expand Down Expand Up @@ -363,3 +364,46 @@ def test_agent_excludes_tool_capabilities_when_no_tools(
type(capability) for capability in agent._root_capability.capabilities
}
assert SkillsCapability not in capability_types


class TestGetAgentCapabilityTools:
"""Tests for get_agent_capability_tools."""

def test_returns_empty_list_when_skills_not_configured(self) -> None:
"""Test that missing skills configuration yields no capability tools."""
assert not get_agent_capability_tools(None)
assert not get_agent_capability_tools(SkillsConfiguration(paths=[]))

def test_returns_skills_tools_when_configured(
self, mock_skills_configuration: SkillsConfiguration
) -> None:
"""Test that configured skills expose pydantic-ai skill tools."""
tools = get_agent_capability_tools(mock_skills_configuration)

assert [tool["identifier"] for tool in tools] == [
"list_skills",
"load_skill",
"read_skill_resource",
"run_skill_script",
]
Comment on lines +383 to +388

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid asserting a non-contractual tool order.

get_agent_capability_tools() preserves capability/tool registration order rather than sorting, so this exact list comparison can fail after harmless upstream refactors even when the returned tool set is correct. Prefer asserting the identifier set, or sort both sides before comparing.

Suggested test adjustment
-        assert [tool["identifier"] for tool in tools] == [
-            "list_skills",
-            "load_skill",
-            "read_skill_resource",
-            "run_skill_script",
-        ]
+        assert {tool["identifier"] for tool in tools} == {
+            "list_skills",
+            "load_skill",
+            "read_skill_resource",
+            "run_skill_script",
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert [tool["identifier"] for tool in tools] == [
"list_skills",
"load_skill",
"read_skill_resource",
"run_skill_script",
]
assert {tool["identifier"] for tool in tools} == {
"list_skills",
"load_skill",
"read_skill_resource",
"run_skill_script",
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/utils/test_pydantic_ai.py` around lines 383 - 388, The test in
test_pydantic_ai should not depend on the registration order returned by
get_agent_capability_tools(), since that order is not contractual. Update the
assertion on the tool identifiers to compare an order-insensitive collection, or
sort both the actual identifiers and the expected list before comparing, using
the get_agent_capability_tools() result and the tool["identifier"] extraction.

assert all(
tool["provider_id"] == "agent-skills"
and tool["toolgroup_id"] == "builtin::agent-skills"
and tool["server_source"] == "builtin"
and tool["type"] == "tool"
for tool in tools
)

load_skill = next(tool for tool in tools if tool["identifier"] == "load_skill")
assert load_skill["parameters"] == [
{
"name": "skill_name",
"description": (
"Exact name from your available skills list.\n"
'Must match exactly (e.g., "data-analysis" not "data analysis").'
),
"parameter_type": "string",
"required": True,
"default": None,
}
]
Loading