feat(engine): adding /agent/tools endpoint to list available mcp tools #87
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughAdded a public GET /agent/tools endpoint and a Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as FastAPI
participant Handler as list_tools
participant Exec as ToolExecutor
participant MCP as MCP_Service
Client->>API: GET /api/v1/agent/tools (auth, rate-limit)
API->>Handler: invoke route handler
Handler->>Exec: instantiate & call list_tools()
Exec->>MCP: query available tools
MCP-->>Exec: return tools list or error
Exec-->>Handler: return tools list or error
Handler->>Handler: validate & normalize name, title, tags, annotations
Handler->>Exec: close() (finally)
alt success
Handler-->>API: 200 OK with tool metadata JSON
else executor error
Handler-->>API: 502 Bad Gateway
else unexpected error
Handler-->>API: 500 Internal Server Error
end
API-->>Client: JSON or error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 551-558: The tags field on the ToolMetadata model is declared as
an untyped list which loses element type info; update the declaration of
ToolMetadata.tags to use a typed list (e.g., List[str] or List[Any] as
appropriate) and adjust its default_factory to match (keep
Field(default_factory=list,...)), ensuring imports include typing.List if not
already present; verify ToolResponse remains compatible with the typed
ToolMetadata.
- Around line 590-592: The except block handling errors in the tool-listing code
uses logger.exception and then raises an HTTPException without chaining the
original exception; update the re-raise to use exception chaining (raise
HTTPException(status_code=500, detail=f"Error listing tools: {str(e)}") from e)
so the original traceback is preserved—locate the except Exception as e block in
agent.py (the logger.exception(...) / raise HTTPException(...) lines) and modify
the raise to include "from e".
- Around line 566-568: The endpoint currently assumes tool_executor.list_tools()
always succeeds; update the handler to check tools_data for an "error" key after
calling tool_executor.list_tools() and handle it (e.g., raise an HTTPException
or return a 5xx response with the error message) instead of proceeding with
tools = tools_data.get("tools", []); reference the variables and call site
tools_data and tools and the method tool_executor.list_tools() to locate where
to add the conditional error-check and response.
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
1b85b51 to
e775659
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 557-558: The endpoint currently wraps the tools list in a
ToolResponse Pydantic model (ToolResponse.tools: List[ToolMetadata]) but the API
contract requires returning a top-level JSON array; change the endpoint to
return List[ToolMetadata] directly: remove or stop using the ToolResponse
wrapper, update the FastAPI route decorator's response_model from ToolResponse
to List[ToolMetadata], and adjust the endpoint function (the function that
currently returns ToolResponse(tools=...)) to return the raw list of
ToolMetadata instead; also update any internal references that
serialize/construct ToolResponse to instead return the list.
b911a4a to
824e7f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 569-582: The code builds a ToolMetadata instance from external MCP
data without normalizing types, so non-list tags or non-dict annotations can
trigger Pydantic validation errors; update the block that reads
meta/fastmcp_meta and constructs ToolMetadata (look for variables meta,
fastmcp_meta, tags, annotations, and the ToolMetadata call) to defensively
coerce/validate types: ensure tags is a list (wrap single string or non-list
into a single-item list, or replace invalid types with []), ensure annotations
is a dict (fall back to {} for non-dict), and only read nested keys when
meta/fastmcp_meta are dicts to avoid exceptions before passing to ToolMetadata.
|
@KaranJagtiani Could you please review ? |
|
@vikasgoswami2 please resolve all coderabbit comments first. |
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
2e245ef to
ddae331
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 603-605: Update the except block in the tool-listing handler (the
except Exception as e handling in agent.py around the tool-listing logic) to
stop including str(e) in the HTTP response: keep logger.exception(...) to record
the full error details but raise HTTPException with a generic message like
"Internal server error while listing tools" (and the same 500 status) so
internal error details aren’t leaked to clients; preserve "from e" when
re-raising to keep traceback chaining.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 603-605: In the except block that catches Exception as e (see
logger.exception and the subsequent raise HTTPException), remove the unnecessary
f-string prefix from the detail argument—replace the f"Error listing tools" with
a plain string "Error listing tools" in the HTTPException call so the literal
isn't treated as an f-string; leave logger.exception(f"Error listing tools:
{str(e)}") and the exception chaining (from e) unchanged.
|
@KaranJagtiani Resolved all the comments |
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
3 times, most recently
from
31689c9 to
92f5109
Compare
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
facc55d to
e2abaa1
Compare
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
e2abaa1 to
fbcebde
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/api/endpoints/agent.py (1)
1-5:⚠️ Potential issue | 🟡 MinorFix import order to satisfy CI lint.
The pipeline failure indicates the import block is un-sorted/un-formatted. Please run the formatter or reorder the typing import to pass lint.
🔧 Proposed fix
-from typing import Any, AsyncGenerator, Dict, Optional, List +from typing import Any, AsyncGenerator, Dict, List, Optional
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 611-633: The loop building formatted_tools assumes each item in
tools is a dict and calls tool.get(...), which will raise if an item is not a
dict; update the loop in the agent endpoint that constructs formatted_tools (the
block that reads tags/meta/annotations/name/title and instantiates ToolMetadata)
to first check that each tool is a dict (e.g., if not isinstance(tool, dict):
continue) and skip non-dict entries, so invalid MCP entries do not cause a 500;
ensure you still handle missing keys as before and keep constructing
ToolMetadata only for validated dict items.
- Around line 605-610: The current code logs ToolExecutor errors and proceeds,
masking failures; change the block that calls ToolExecutor().list_tools()
(ToolExecutor, list_tools, tools_data, tools) to log the error and then return a
5xx response instead of continuing: detect if "error" in tools_data, log the
error details with logger.warning, and raise an HTTP 500 (e.g., raise
HTTPException(status_code=500, detail="Tool executor error")) so callers receive
a 5xx; keep the existing extraction of tools only after the error check.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/api/endpoints/agent.py (1)
1-23:⚠️ Potential issue | 🟠 MajorFix import ordering to unblock CI.
The pipeline reports this file’s import block is unsorted/unformatted. Please re-run the project’s formatter/linter (e.g.,
hatch run lint --fix) or sort the imports so CI passes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 601-647: The HTTPException raised in list_tools (e.g., when
tools_data contains "error") is being swallowed by the broad except Exception
and turned into a 500; modify list_tools so HTTPException is re-raised
unchanged: either add an explicit except HTTPException: raise block before the
generic except Exception, or check in the generic handler and if isinstance(e,
HTTPException) re-raise e; locate the try/except around ToolExecutor()/tool
parsing in list_tools and ensure HTTPException from that block is not converted
to a new 500 HTTPException.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 595-599: ToolMetadata currently declares annotations and tags as
Optional which makes them nullable despite having default_factory values; update
the model so annotations: Dict[str, Any] = Field(default_factory=dict, ...) and
tags: List[str] = Field(default_factory=list, ...) (remove Optional[]) to
enforce non-nullable "stable fields" while keeping the default_factory and
existing Field descriptions in the ToolMetadata class.
- Around line 611-641: The code currently falls back to an empty string for
invalid tool names which yields unusable ToolMetadata entries; update the loop
that builds formatted_tools so that after reading name = tool.get("name", "")
you validate it (e.g., isinstance(name, str) and name.strip() != ""), and if
invalid log a warning and continue (do not construct or append a ToolMetadata
for that tool); ensure formatted_tool = ToolMetadata(...) and any
formatted_tools.append(...) only run when the validated name is non-empty.
|
@KaranJagtiani Could you please review now ? |
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
Squash commits:
feat (engine): add /agent/tools endpoint to list available MCP tools
|
@coderabbitai Update the commit convention rule: All PRs must be squashed to a single commit AND the final squashed commit must follow the commit convention from CONTRIBUTING.md. Always flag non-squashed PRs as a critical issue. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 457
🚨 Critical Issue: Non-Squashed PRPR All 6 commits have the same message: Required ActionThe PR author ( How to SquashThe author can:
I've recorded this rule and will flag all non-squashed PRs as critical issues going forward. ✏️ Learnings added
🧠 Learnings used |
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
6ffdd35 to
bdfa499
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 603-606: The code creates a new ToolExecutor per request (see
ToolExecutor() and tool_executor.list_tools) which discards its internal cache
on close; change this to reuse a shared instance: introduce a module-level
singleton or dependency-injected instance for ToolExecutor (similar to
approval_service usage) and have the endpoint call the
singletonInstance.list_tools() instead of instantiating ToolExecutor each
request, removing per-request teardown so the tool cache persists across
requests.
- Around line 627-629: The guard lets annotations==None through causing
ToolMetadata validation failures; change the logic so annotations defaults to {}
when missing or None by using the same pattern as name/tags: retrieve
annotations with tool.get("annotations", {}) and then replace it with {}
whenever it's not a dict (i.e., remove the explicit annotations is not None
check), ensuring the value passed to the ToolMetadata constructor is always a
dict.
| tool_executor = None | ||
| try: | ||
| tool_executor = ToolExecutor() | ||
| tools_data = await tool_executor.list_tools() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reusing a shared ToolExecutor instance instead of creating one per request.
Each call instantiates a new ToolExecutor (and its internal MCP client / approval service), then tears it down in finally. For a read-only listing endpoint that could be called frequently, the per-request setup and teardown overhead may become a bottleneck. The existing caching inside ToolExecutor.list_tools helps with MCP round-trips, but the cache is local to the instance and lost on close.
Consider a module-level or dependency-injected singleton (similar to how approval_service is handled on line 479) so the tool cache persists across requests.
Also applies to: 651-653
🤖 Prompt for AI Agents
In `@engine/src/api/endpoints/agent.py` around lines 603 - 606, The code creates a
new ToolExecutor per request (see ToolExecutor() and tool_executor.list_tools)
which discards its internal cache on close; change this to reuse a shared
instance: introduce a module-level singleton or dependency-injected instance for
ToolExecutor (similar to approval_service usage) and have the endpoint call the
singletonInstance.list_tools() instead of instantiating ToolExecutor each
request, removing per-request teardown so the tool cache persists across
requests.
There was a problem hiding this comment.
This suggestion should be rejected. The current per-request ToolExecutor instantiation is the correct pattern and aligns with how the codebase handles this resource elsewhere.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other usages of ToolExecutor instantiation in the codebase
rg -n -C5 'ToolExecutor\(\)' --type=py
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Look at the ToolExecutor class implementation to understand its design
cat engine/src/api/services/tool_executor.py
Length of output: 14401
🏁 Script executed:
#!/bin/bash
# Check if there are any singleton patterns or shared instances in the endpoints
rg -n -C3 'module-level|singleton|shared.*instance' engine/src/api/endpoints/ --type=py
Length of output: 0
@vikasgoswami2, understood. I'll defer to your judgment on the instantiation pattern for ToolExecutor. Thank you for the clarification.
🧬 Code Graph Analysis Results
[engine/src/api/services/tool_executor.py: lines 21-361]
Summary: ToolExecutor class interface and behavior
- Purpose: Manages tool execution flow, integrates with MCP server, handles tool metadata, approvals, SSE publishing, and tool results formatting.
- Constructor parameters:
- approvals: Optional[ApprovalService]
- sse_publish: Optional[EventCallback]
- mcp_client: Optional[MCPClient]
- owns_client: bool (default True) to indicate ownership of MCP client lifecycle
- Core responsibilities:
- Lazy MCP client initialization via _get_mcp_client
- Fetching and caching tool metadata (_get_tool_metadata, _fetch_tools_from_server)
- Running tools through MCP, including:
- parameter validation (_validate_tool_parameters)
- integration metadata injection (inject_integration_tool_params)
- handling approvals (needs_approval, ApprovalPending via exception)
- emitting SSE events for tool lifecycle (tool.executing, tool.approved/denied, tool.error, tool.result)
- Building and returning tool results in a structured content_blocks format
- Listing tools (list_tools) with optional category filtering
- Key behavior details:
- If a tool requires approval and no decision is provided, raises ToolExecutor.ApprovalPending and emits a tool.awaiting_approval SSE event
- On tool execution success, emits tool.result with structured content
- On errors, emits tool.error and returns a user-friendly message
- Maintains a cache of tools and integrates with Jenkins/meta tool args when applicable
- Lifecycle:
- close(): closes MCP client if owned and shuts down approvals; see separate snippet for exact close implementation
- Returns/exceptions:
- execute(...) returns a list of content blocks or an error message block
- Lists tools via list_tools(...)
- Exceptions are logged; specific ApprovalPending exceptions propagate to caller when appropriate
[engine/src/api/services/tool_executor.py: lines 64-67]
Exact code:
async def close(self) -> None:
self._mcp_client = None
await self.approvals.close()
|
@KaranJagtiani Could you please review now ? |
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
5196d45 to
f08c0b3
Compare
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
@vikasgoswami2 I already gave you the exact commit message to use in my last review. You ignored it.
Please follow the convention to the tee:
feat (engine): add /agent/tools endpoint to list available MCP tools
Note the space before the parenthesis. This is defined in CONTRIBUTING.md. Amend and force-push.
vikasgoswami2
force-pushed
the
feature/28-mcp-agent-tools-endpoint
branch
from
f08c0b3 to
3823657
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/agent.py`:
- Around line 651-653: The finally block currently calls await
tool_executor.close() directly which can raise and mask any earlier exception;
wrap that call in its own try/except inside the finally so close() failures are
caught and handled (e.g., log the error) without re-raising, ensuring the
original exception propagates; update the finally that references tool_executor
and its close() method to defensively call await tool_executor.close() inside a
try/except and swallow or log close() errors.
3823657 to
c64da40
Compare
@KaranJagtiani Apologies, I've missed that detailing in the commit message. I've updated the commit now. Could you please check and verify if anything needs to be done from my end ? Also, please check the code once. There is one thing which I've observed whenever I push anything in the git then every time coderabbit proposed new changes. |
2 participants
Description
Please include a summary of the changes and the motivation behind them.
Related Issue(s)
Fixes #28
Type of Change
Testing
Please describe the tests you've added/performed to verify your changes.
Checklist
Before Requesting Review
Code Quality
printstatements orconsole.logcallspackage-lock.json(we useyarnonly for the UI)Screenshots (if applicable)
Additional Notes