LCORE-2853: skills impact /tools#2024
Conversation
WalkthroughAdds Agent 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
1 participant
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