-
Notifications
You must be signed in to change notification settings - Fork 94
LCORE-2853: skills impact /tools
#2024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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", | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 As per coding guidelines, 🤖 Prompt for AI AgentsSources: 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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, Also applies to: 195-203, 232-241 🤖 Prompt for AI AgentsSources: 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, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| LlamaStackConfiguration, | ||
| ModelContextProtocolServer, | ||
| ServiceConfiguration, | ||
| SkillsConfiguration, | ||
| TLSConfiguration, | ||
| UserDataCollection, | ||
| ) | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||||||||||||||||||||||
| _model_settings_from_responses_params, | ||||||||||||||||||||||||||
| _skills_capability, | ||||||||||||||||||||||||||
| build_agent, | ||||||||||||||||||||||||||
| get_agent_capability_tools, | ||||||||||||||||||||||||||
| llama_stack_provider_from_client, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||
There was a problem hiding this comment.
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/toolsalready carriesprovider_idandtoolgroup_idbecause same-named tools can exist in different sources. With the current logic, an MCP or builtin tool namedlist_skills/load_skillwill silently suppress the capability tool, which defeats the discoverability this PR is adding.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents