LCORE-2853: skills impact /tools#2024
Conversation
WalkthroughAdds capability-tool serialization from configured skills and appends those tools to the ChangesAgent Capability Tools Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/app/endpoints/tools.py`:
- Around line 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.
In `@src/utils/pydantic_ai.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19ce1df6-12a5-4097-89b0-f17c45999d1b
📒 Files selected for processing (4)
src/app/endpoints/tools.pysrc/utils/pydantic_ai.pytests/unit/app/endpoints/test_tools.pytests/unit/utils/test_pydantic_ai.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
⚠️ CI failures not shown inline (2)
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-2853: skills impact /tools
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-2853: skills impact /tools
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/app/endpoints/tools.pysrc/utils/pydantic_ai.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/tools.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/app/endpoints/test_tools.pytests/unit/utils/test_pydantic_ai.py
🧠 Learnings (2)
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/tools.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
src/app/endpoints/tools.pytests/unit/app/endpoints/test_tools.pytests/unit/utils/test_pydantic_ai.pysrc/utils/pydantic_ai.py
| 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) |
There was a problem hiding this comment.
🎯 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.
| 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.
| 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) |
There was a problem hiding this comment.
📐 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
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/utils/pydantic_ai.py`:
- Around line 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.
In `@tests/unit/app/endpoints/test_tools.py`:
- Around line 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.
In `@tests/unit/utils/test_pydantic_ai.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8e785e2-4fbb-4999-8bec-d5876baa4e1b
📒 Files selected for processing (4)
src/app/endpoints/tools.pysrc/utils/pydantic_ai.pytests/unit/app/endpoints/test_tools.pytests/unit/utils/test_pydantic_ai.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: integration_tests (3.13)
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
⚠️ CI failures not shown inline (2)
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-2853: skills impact /tools
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-2853: skills impact /tools
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/utils/test_pydantic_ai.pytests/unit/app/endpoints/test_tools.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/app/endpoints/tools.pysrc/utils/pydantic_ai.py
src/app/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/app/**/*.py: FastAPI dependencies: Import fromfastapimodule forAPIRouter,HTTPException,Request,status,Depends
Use FastAPIHTTPExceptionwith appropriate status codes for API endpoints and handleAPIConnectionErrorfrom Llama Stack
Files:
src/app/endpoints/tools.py
🧠 Learnings (2)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/unit/utils/test_pydantic_ai.pytests/unit/app/endpoints/test_tools.pysrc/app/endpoints/tools.pysrc/utils/pydantic_ai.py
📚 Learning: 2026-04-06T20:18:07.852Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1463
File: src/app/endpoints/rlsapi_v1.py:266-271
Timestamp: 2026-04-06T20:18:07.852Z
Learning: In the lightspeed-stack codebase, within `src/app/endpoints/` inference/MCP endpoints, treat `tools: Optional[list[Any]]` in MCP tool definitions as an intentional, consistent typing pattern (used across `query`, `responses`, `streaming_query`, `rlsapi_v1`). Do not raise or suggest this as a typing issue during code review; changing it in isolation could break endpoint typing consistency across the codebase.
Applied to files:
src/app/endpoints/tools.py
🔇 Additional comments (4)
src/utils/pydantic_ai.py (2)
105-145: Duplicate: expand these helper docstrings to the repo format.These helper docstrings are still one-line summaries; the earlier review already flagged adding
Parameters:/Returns:/Raises:sections here. As per coding guidelines, functions undersrc/**/*.pymust include descriptive docstrings with required sections. Based on learnings, this repo usesParameters:rather thanArgs:.Sources: Coding guidelines, Learnings
5-6: LGTM!Also applies to: 22-26
src/app/endpoints/tools.py (2)
242-250: Duplicate: deduplicate capability tools by source identity, not name alone.This block still keys only on
identifier, so a same-named MCP or built-in tool can suppress an agent-skills tool even though the response carriesprovider_idandtoolgroup_id. The previous review already flagged this exact merge logic.
31-31: LGTM!Also applies to: 252-260
| 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. | ||
| """ |
There was a problem hiding this comment.
📐 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
| 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" |
There was a problem hiding this comment.
📐 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.
| assert [tool["identifier"] for tool in tools] == [ | ||
| "list_skills", | ||
| "load_skill", | ||
| "read_skill_resource", | ||
| "run_skill_script", | ||
| ] |
There was a problem hiding this comment.
📐 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.
| 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.
3 participants
Description
When agent skills are enabled, pydantic-ai registers capability tools (
list_skills,load_skill,read_skill_resource,run_skill_script) that agents use internally. The/toolsendpoint previously only surfaced Llama Stack toolgroups and MCP server tools, so clients could not discover these skills-related tools.This PR adds
get_agent_capability_tools()to serialize pydantic-ai capability toolsets into the same format as the/toolsresponse, and merges them into the consolidated tools list in the endpoint handler (with deduplication by identifier). Skills tools are tagged withprovider_id: agent-skills,toolgroup_id: builtin::agent-skills, andserver_source: builtin.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
tests/unit/utils/test_pydantic_ai.pyforget_agent_capability_tools()(empty when skills disabled, correct tools/metadata when configured)tests/unit/app/endpoints/test_tools.pyverifying/toolsincludes capability tools when skills are configured/toolsendpoint with skills enabled and confirmedlist_skills,load_skill,read_skill_resource, andrun_skill_scriptappear with expected metadataSummary by CodeRabbit
New Features
Bug Fixes
Tests