LCORE-202: Add GET /v1/tools endpoint to list tools from MCP servers #626
Conversation
WalkthroughAdds a new protected GET /v1/tools endpoint that aggregates toolgroups and tools from configured MCP servers and built‑in groups, formats results via a new formatter, returns a ToolsResponse, registers the router, and adds Action.GET_TOOLS. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as FastAPI /v1/tools
participant Auth as Auth Dependency
participant Cfg as Config Loader
participant Holder as AsyncLlamaStackClientHolder
participant Llama as Llama Stack Client
participant Util as Tool Formatter
Client->>API: GET /v1/tools
API->>Auth: validate auth
Auth-->>API: AuthTuple
API->>Cfg: ensure config (MCP servers)
API->>Holder: acquire client
alt client acquired
Holder-->>API: client
API->>Llama: list toolgroups
alt toolgroups retrieved
loop for each toolgroup
API->>Llama: list tools for toolgroup
alt tools retrieved
loop for each tool
API->>Util: format_tool_response(tool)
Util-->>API: formatted tool
end
else per-toolgroup error
Llama-->>API: error (logged) and continue
end
end
API-->>Client: 200 ToolsResponse (tools=[...])
else APIConnectionError
Llama--x API: APIConnectionError
API-->>Client: 500 JSON error (connection details)
else other error
Llama--x API: Exception
API-->>Client: 500 JSON error (retrieval error)
end
else holder error
Holder--x API: Exception
API-->>Client: 500 JSON error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/tools.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)src/utils/tool_formatter.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.pysrc/models/responses.pysrc/app/endpoints/tools.pysrc/utils/tool_formatter.pysrc/app/routers.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/models/config.pysrc/models/responses.pysrc/app/endpoints/tools.pysrc/utils/tool_formatter.pysrc/app/routers.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/config.pysrc/models/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/config.pysrc/models/responses.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/tools.pysrc/app/routers.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/tools.pysrc/app/routers.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/tools.py
🧬 Code graph analysis (2)
src/app/endpoints/tools.py (8)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)mcp_servers(93-97)src/models/config.py (2)
config(138-144)Action(327-367)src/models/responses.py (1)
ToolsResponse(39-42)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)src/utils/tool_formatter.py (1)
format_tools_list(76-86)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(32-47)
🪛 GitHub Actions: Black
src/models/responses.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black.
src/app/endpoints/tools.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black.
src/utils/tool_formatter.py
[error] 1-1: Black formatting check failed. This file would be reformatted by Black.
🪛 GitHub Actions: Pyright
src/app/endpoints/tools.py
[error] 121-121: "tool_dict" is possibly unbound (reportPossiblyUnboundVariable) [uv run pyright src]
🪛 GitHub Actions: Python linter
src/models/responses.py
[error] 42-42: Line too long (110/100) (line-too-long)
src/app/endpoints/tools.py
[warning] 88-88: Trailing whitespace (trailing-whitespace)
[warning] 90-90: Line too long (128/100) (line-too-long)
[warning] 91-91: Trailing whitespace (trailing-whitespace)
[warning] 96-96: Trailing whitespace (trailing-whitespace)
[warning] 101-101: Trailing whitespace (trailing-whitespace)
[warning] 105-105: Trailing whitespace (trailing-whitespace)
[warning] 109-109: Line too long (125/100) (line-too-long)
[warning] 114-114: Trailing whitespace (trailing-whitespace)
[warning] 116-116: Trailing whitespace (trailing-whitespace)
[warning] 119-119: Trailing whitespace (trailing-whitespace)
[warning] 123-123: Trailing whitespace (trailing-whitespace)
[warning] 127-127: Trailing whitespace (trailing-whitespace)
[warning] 131-131: Trailing whitespace (trailing-whitespace)
[warning] 134-134: Trailing whitespace (trailing-whitespace)
[warning] 135-135: Line too long (101/100) (line-too-long)
[warning] 135-135: Trailing whitespace (trailing-whitespace)
[warning] 132-132: Catching too general exception 'Exception' (broad-exception-caught)
[warning] 124-124: Catching too general exception 'Exception' (broad-exception-caught)
src/utils/tool_formatter.py
[warning] 25-25: Trailing whitespace (trailing-whitespace)
[warning] 36-36: Trailing whitespace (trailing-whitespace)
[warning] 61-61: Trailing whitespace (trailing-whitespace)
[warning] 67-67: Trailing whitespace (trailing-whitespace)
[warning] 70-70: Trailing whitespace (trailing-whitespace)
[warning] 87-87: Trailing newline (trailing-newlines)
[warning] 71-71: Catching too general exception 'Exception' (broad-exception-caught)
[warning] 3-3: Unused import 're' (unused-import)
[warning] 5-5: Unused import 'Optional' from typing (unused-import)
🪛 GitHub Actions: Ruff
src/utils/tool_formatter.py
[error] 3-3: Ruff: 're' imported but unused. (F401)
[error] 5-5: Ruff: 'typing.Optional' imported but unused. (F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
bsatapat-jpg
force-pushed
the
tools_mcp
branch
from
f000ac5 to
812c991
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/endpoints/tools.py (1)
6-7: ImportDependsfromfastapiper project conventions.The module imports
Dependsfromfastapi.params, but the coding guidelines specify: "Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends)".As per coding guidelines, apply this diff:
-from fastapi import APIRouter, HTTPException, Request, status -from fastapi.params import Depends +from fastapi import APIRouter, Depends, HTTPException, Request, status
🧹 Nitpick comments (1)
src/app/endpoints/tools.py (1)
109-142: Consider clarifying theserver_sourcecapture pattern.Line 135 assigns
server_sourcefrom each tool, leaving it with the last tool's value for logging. While this works correctly (all tools in a toolgroup share the same source), the pattern is a bit unclear.For clarity, consider determining
server_sourceonce before the tool loop:# Convert tools to dict format tools_count = 0 - server_source = "unknown" + + # Determine server source for this toolgroup + if toolgroup.identifier in mcp_server_names: + mcp_server = next( + (s for s in configuration.mcp_servers if s.name == toolgroup.identifier), + None, + ) + server_source = mcp_server.url if mcp_server else toolgroup.identifier + else: + server_source = "builtin" for tool in tools_response: tool_dict = dict(tool) - # Determine server source based on toolgroup type - if toolgroup.identifier in mcp_server_names: - # This is an MCP server toolgroup - mcp_server = next( - ( - s - for s in configuration.mcp_servers - if s.name == toolgroup.identifier - ), - None, - ) - tool_dict["server_source"] = ( - mcp_server.url if mcp_server else toolgroup.identifier - ) - else: - # This is a built-in toolgroup - tool_dict["server_source"] = "builtin" + tool_dict["server_source"] = server_source consolidated_tools.append(tool_dict) tools_count += 1 - server_source = tool_dict["server_source"]This eliminates redundant lookups and makes the logging intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/endpoints/tools.py(1 hunks)src/models/responses.py(1 hunks)src/utils/tool_formatter.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/tool_formatter.py
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/app/test_routers.pysrc/models/responses.pysrc/app/endpoints/tools.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/test_routers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/app/test_routers.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/responses.pysrc/app/endpoints/tools.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/responses.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/tools.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/tools.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/tools.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/**/*.py : Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Applied to files:
src/app/endpoints/tools.py
🧬 Code graph analysis (1)
src/app/endpoints/tools.py (8)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)mcp_servers(93-97)src/models/config.py (2)
config(138-144)Action(327-367)src/models/responses.py (1)
ToolsResponse(39-44)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)src/utils/tool_formatter.py (1)
format_tools_list(87-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (8)
src/models/responses.py (1)
39-44: LGTM! The formatting issue has been addressed.The
ToolsResponsemodel is well-structured with proper type hints and a clear description. TheFieldcall is now correctly wrapped across multiple lines, resolving the previous line-length violation.tests/unit/app/test_routers.py (3)
22-22: LGTM! Import addition is correct.The
toolsendpoint module is properly imported alongside other endpoint modules.
65-77: LGTM! Test assertions correctly validate the new router.The router count is appropriately incremented to 13, and the presence of
tools.routeris verified. This is consistent with the test structure for other endpoint routers.
86-98: LGTM! Prefix validation is correct.The test correctly verifies that
tools.routeris mounted with the/v1prefix, consistent with other API v1 endpoints likemodelsandquery.src/app/endpoints/tools.py (4)
24-54: LGTM! Response schema is well-defined.The example response structure clearly documents the expected format, including all relevant fields like
identifier,parameters,server_source, etc. The 200 and 500 status code descriptions are appropriate.
57-82: LGTM! Endpoint signature and documentation are correct.The endpoint properly:
- Uses the
@authorizedecorator withAction.GET_TOOLS- Includes type hints for all parameters and return type
- Has a comprehensive docstring following Google style
- Marks unused variables appropriately
85-165: LGTM! Main logic correctly aggregates tools from multiple sources.The implementation properly:
- Retrieves all toolgroups from Llama Stack
- Iterates through each toolgroup to get its tools
- Classifies tools as MCP or builtin based on toolgroup identifier
- Formats tools using the utility function
- Handles per-toolgroup failures gracefully to continue processing
The error handling strategy (continuing on per-toolgroup failures at line 144-150, and continuing with partial results on toolgroups.list() failure at lines 152-153) ensures the endpoint remains resilient and returns available tools even if some sources fail.
167-186: LGTM! Exception handling provides appropriate error responses.The endpoint handles errors properly:
- Specifically catches
APIConnectionErrorfor Llama Stack connectivity issues (lines 168-176)- Falls back to a general exception handler for unexpected errors (lines 178-186)
- Both return structured JSON error responses with appropriate HTTP 500 status
- All errors are properly logged before raising HTTPException
The broad
Exceptioncatch at the outermost level is acceptable as a safety net to ensure the API returns a structured error response rather than an unhandled exception.
bsatapat-jpg
force-pushed
the
tools_mcp
branch
from
12a02c1 to
bfb8d9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/models/responses.py (1)
39-41: Enhance docstring to follow coding guidelines.Per coding guidelines, class docstrings should follow Google Python style with Attributes and Example sections. Other response models in this file (e.g.,
InfoResponse,ConversationResponse) demonstrate this pattern.Apply this diff to enhance the docstring:
class ToolsResponse(BaseModel): - """Model representing a response to tools request.""" + """Model representing a response to tools request. + + Attributes: + tools: List of tools available from all configured MCP servers. + + Example: + ```python + tools_response = ToolsResponse( + tools=[ + { + "name": "web_search", + "description": "Search the web for information", + "input_schema": { + "type": "object", + "properties": { + "query": {"type": "string"} + } + } + } + ] + ) + ``` + """Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)tests/unit/app/test_routers.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/routers.py
- tests/unit/app/test_routers.py
- src/models/config.py
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/responses.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/models/responses.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/responses.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
bsatapat-jpg
force-pushed
the
tools_mcp
branch
4 times, most recently
from
5bd13d3 to
d015bf3
Compare
bsatapat-jpg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/tools.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)tests/unit/app/test_routers.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/models/responses.py
- src/models/config.py
- tests/unit/app/test_routers.py
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/tools.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/tools.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/app/endpoints/tools.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/tools.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/tools.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/**/*.py : Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Applied to files:
src/app/endpoints/tools.py
🧬 Code graph analysis (1)
src/app/endpoints/tools.py (8)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)mcp_servers(93-97)src/models/config.py (2)
config(138-144)Action(327-368)src/models/responses.py (1)
ToolsResponse(39-67)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)src/utils/tool_formatter.py (1)
format_tools_list(87-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
bsatapat-jpg
changed the title
bsatapat-jpg
force-pushed
the
tools_mcp
branch
2 times, most recently
from
e06bf4b to
2f8e42c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/app/endpoints/tools.py (1)
109-109: Unused initialization ofserver_source.The
server_source = "unknown"initialization is immediately overwritten in the loop on lines 125-130, making this assignment unnecessary. Consider removing it or documenting why it's needed (e.g., for edge cases where the loop doesn't execute).Apply this diff to remove the unused initialization:
# Convert tools to dict format tools_count = 0 - server_source = "unknown" for tool in tools_response:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/tools.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)tests/unit/app/endpoints/test_tools.py(1 hunks)tests/unit/app/test_routers.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/models/config.py
- src/models/responses.py
- tests/unit/app/test_routers.py
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/tools.pysrc/app/routers.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/tools.pysrc/app/routers.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/app/endpoints/tools.pysrc/app/routers.pytests/unit/app/endpoints/test_tools.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/tools.pysrc/app/routers.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/tools.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/endpoints/test_tools.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/app/endpoints/test_tools.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/**/*.py : Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Applied to files:
src/app/endpoints/tools.py
🧬 Code graph analysis (2)
src/app/endpoints/tools.py (8)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)mcp_servers(93-97)src/models/config.py (2)
config(138-144)Action(327-368)src/models/responses.py (1)
ToolsResponse(39-67)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)src/utils/tool_formatter.py (1)
format_tools_list(87-97)
src/app/routers.py (1)
tests/unit/app/test_routers.py (1)
include_router(34-49)
🪛 GitHub Actions: Black
tests/unit/app/endpoints/test_tools.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by 'black --check'. Run 'black' to format the code.
🪛 GitHub Actions: Python linter
tests/unit/app/endpoints/test_tools.py
[warning] 409-409: pylint: Trailing whitespace (trailing-whitespace)
[warning] 413-413: pylint: Trailing whitespace (trailing-whitespace)
[warning] 102-102: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 102-102: pylint: Redefining name 'mock_tools_response' from outer scope (redefined-outer-name)
[warning] 105-105: pylint: Unused variable 'mock_config' (unused-variable)
[warning] 196-196: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 234-234: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 234-234: pylint: Redefining name 'mock_tools_response' from outer scope (redefined-outer-name)
[warning] 279-279: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 419-419: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 448-448: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 478-478: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 504-504: pylint: Redefining name 'mock_configuration' from outer scope (redefined-outer-name)
[warning] 4-4: pylint: standard import "unittest.mock.AsyncMock" should be placed before third party import "pytest" (wrong-import-order)
🪛 GitHub Actions: Ruff
tests/unit/app/endpoints/test_tools.py
[error] 105-105: F841 Local variable mock_config is assigned to but never used. Remove assignment to unused variable mock_config.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (9)
src/app/endpoints/tools.py (6)
1-21: LGTM!Module setup follows all coding guidelines: descriptive docstring, standard FastAPI imports, proper logger initialization, and clean router definition.
23-53: LGTM!Response schema is well-defined with appropriate status codes and example structure that matches the ToolsResponse model.
56-75: LGTM!Handler signature follows conventions: async def for I/O operations, proper authorization decorator, complete type annotations, and Google-style docstring with all required sections.
153-158: Verify behavior whentoolgroups.list()raises ValueError/AttributeError.When
toolgroups.list()raisesValueErrororAttributeError, the exception is caught and logged (line 157) but not re-raised. This causes the endpoint to return HTTP 200 with an empty tools list instead of HTTP 500. Is this the intended behavior for these error types, or should they also propagate to return HTTP 500?For comparison,
APIConnectionErroron line 153 is correctly re-raised to trigger the outer handler's HTTP 500 response.Consider whether
ValueErrorandAttributeErrorfromtoolgroups.list()represent recoverable scenarios (empty result acceptable) or unrecoverable errors (should return 500). If they should return 500, re-raise them:except APIConnectionError as e: logger.warning("Failed to retrieve tools from toolgroups: %s", e) raise except (ValueError, AttributeError) as e: logger.warning("Failed to retrieve tools from toolgroups: %s", e) + raise
84-169: Well-structured partial failure handling.The three-tiered error handling elegantly supports partial failures:
- Inner try-except (lines 101-151) continues processing other toolgroups when one fails
- Middle try-except (lines 96-158) handles
toolgroups.list()failures- Outer try-except (lines 172-190) converts exceptions to structured HTTP 500 responses
This design allows the endpoint to return available tools even when some MCP servers are unreachable.
171-190: LGTM!Exception handlers follow FastAPI best practices: appropriate HTTP 500 status codes, structured error details with
responseandcausefields, and proper exception chaining withfrom e.src/app/routers.py (2)
19-19: LGTM!The tools endpoint import is correctly placed with other endpoint imports.
32-32: LGTM!Router registration follows the established pattern with the
/v1prefix, consistent with other endpoint registrations.tests/unit/app/endpoints/test_tools.py (1)
21-527: Comprehensive test coverage.The test suite thoroughly covers the endpoint's behavior:
- Successful aggregation from multiple MCP servers
- Empty MCP server configurations
- API connection errors (both at toolgroup and individual server level)
- Partial failures with graceful degradation
- Built-in toolgroups vs. MCP toolgroups
- Mixed scenarios
- Various exception types
The use of
__wrapped__to bypass decorators andAsyncMockfor async operations follows pytest best practices. The redefined fixture name warnings from pylint are false positives—pytest intentionally allows fixtures as function parameters.Note: Run
black tests/unit/app/endpoints/test_tools.pyto address the Black formatting issue flagged in the pipeline.
bsatapat-jpg
force-pushed
the
tools_mcp
branch
3 times, most recently
from
fd5bdbe to
8438aca
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/tools.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)tests/unit/app/endpoints/test_tools.py(1 hunks)tests/unit/app/test_routers.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/models/config.py
- src/models/responses.py
- src/app/endpoints/tools.py
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/app/endpoints/test_tools.pytests/unit/app/test_routers.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/app/endpoints/test_tools.pytests/unit/app/test_routers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/app/endpoints/test_tools.pytests/unit/app/test_routers.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
bsatapat-jpg
force-pushed
the
tools_mcp
branch
from
8438aca to
36008c4
Compare
|
bsatapat-jpg
force-pushed
the
tools_mcp
branch
from
36008c4 to
7c1ca14
Compare
bsatapat-jpg
force-pushed
the
tools_mcp
branch
from
7c1ca14 to
0393a96
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/responses.py (1)
39-67: Add model_config for API documentation consistency.The
ToolsResponsemodel lacks themodel_configwithjson_schema_extrathat is present in all other response models (e.g.,ModelsResponse,ShieldsResponse,QueryResponse). This omission means the/docsendpoint will display less helpful examples for the/toolsendpoint.Add the following after the
toolsfield definition:# provides examples for /docs endpoint model_config = { "json_schema_extra": { "examples": [ { "tools": [ { "identifier": "filesystem_read", "description": "Read contents of a file from the filesystem", "parameters": [ { "name": "path", "description": "Path to the file to read", "parameter_type": "string", "required": True, "default": None, } ], "provider_id": "model-context-protocol", "toolgroup_id": "filesystem-tools", "server_source": "http://localhost:3000", "type": "tool", } ] } ] } }src/app/endpoints/tools.py (1)
159-164: Optional: Optimize summary logging to avoid multiple passes.The current logging iterates
consolidated_toolsthree times. While this is not a performance issue for typical tool list sizes, you could compute the counts in a single pass:builtin_count = sum(1 for t in consolidated_tools if t.get("server_source") == "builtin") mcp_count = len(consolidated_tools) - builtin_count logger.info( "Retrieved total of %d tools (%d from built-in toolgroups, %d from MCP servers)", len(consolidated_tools), builtin_count, mcp_count, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/endpoints/tools.py(1 hunks)src/app/routers.py(2 hunks)src/models/config.py(1 hunks)src/models/responses.py(1 hunks)src/utils/tool_formatter.py(1 hunks)tests/unit/app/endpoints/test_tools.py(1 hunks)tests/unit/app/test_routers.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/app/test_routers.py
- src/models/config.py
- tests/unit/app/endpoints/test_tools.py
- src/app/routers.py
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/tools.pysrc/models/responses.pysrc/utils/tool_formatter.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/tools.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/app/endpoints/tools.pysrc/models/responses.pysrc/utils/tool_formatter.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/tools.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/tools.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/responses.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/responses.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/app/**/*.py : Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Applied to files:
src/app/endpoints/tools.py
🧬 Code graph analysis (1)
src/app/endpoints/tools.py (8)
src/authentication/__init__.py (1)
get_auth_dependency(14-52)src/authorization/middleware.py (1)
authorize(111-122)src/client.py (2)
AsyncLlamaStackClientHolder(18-55)get_client(49-55)src/configuration.py (2)
configuration(65-69)mcp_servers(93-97)src/models/config.py (2)
config(138-144)Action(327-368)src/models/responses.py (1)
ToolsResponse(39-67)src/utils/endpoints.py (1)
check_configuration_loaded(68-80)src/utils/tool_formatter.py (1)
format_tools_list(98-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (2)
src/utils/tool_formatter.py (1)
1-108: LGTM!The utility functions are well-structured with proper docstrings, complete type annotations, and appropriate error handling. The use of named constants for magic numbers and specific exception catching addresses previous review feedback effectively.
src/app/endpoints/tools.py (1)
1-190: LGTM!The endpoint implementation is well-structured with proper authorization, error handling, and documentation. The use of nested try-except blocks appropriately handles partial failures (individual toolgroups can fail without breaking the entire response) while treating connection errors as fatal. The error handling strategy aligns with the documented behavior.
|
Hi @tisnik, I have resolved all the feedback. PTAL. |
2 participants
Description
I implemented a new REST API endpoint /v1/tools that retrieves a consolidated list of tools from all configured MCP (Model Context Protocol) servers. The implementation went through several iterations to address user requirements and fix issues.
Implementation
New Files Created:
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Error Handling
Utilities
Tests