feat(mcp/tools): add shell_command, document_analyzer, semantic_search, and ask_agent tools to MCP registry#401
feat(mcp/tools): add shell_command, document_analyzer, semantic_search, and ask_agent tools to MCP registry#401
Conversation
…h, and ask_agent tools to MCP registry
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the MCP (Multi-Codebase Project) tool registry by integrating several new tools for advanced code interaction and analysis. It introduces functionalities for executing shell commands, analyzing documents, performing semantic searches, and retrieving function source code. A key addition is the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR extends Key concerns:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Registry as MCPToolsRegistry
participant Agent as RAG Agent (lazy)
participant Tools as Registered Tools
Client->>Registry: ask_agent(question)
Registry->>Registry: redirect stdout/stderr (global!)
Registry->>Registry: logger.disable("codebase_rag") (global!)
Registry->>Agent: rag_agent.run(question, message_history=[])
Agent->>Tools: query_code_graph / get_code_snippet / shell_command / etc.
Tools-->>Agent: results
Agent-->>Registry: response.output
Registry->>Registry: logger.enable("codebase_rag") (finally)
Registry-->>Client: {"output": response.output}
note over Registry: If exception thrown, swallowed silently —<br/>no log, generic error message returned
Last reviewed commit: 665aad3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with redirect_stdout(io.StringIO()), redirect_stderr(io.StringIO()): | ||
| # Temporarily disable loguru logging | ||
| logger.disable("codebase_rag") | ||
| try: | ||
| # Run the query using the RAG agent | ||
| response = await self.rag_agent.run(question, message_history=[]) | ||
| return {"output": response.output} | ||
| finally: | ||
| # Re-enable logging | ||
| logger.enable("codebase_rag") |
There was a problem hiding this comment.
Global logger suppression is not async-safe
logger.disable("codebase_rag") modifies loguru's global state, which is shared across all coroutines. In a concurrent async server, if two ask_agent calls are in-flight simultaneously, coroutine A's logger.disable(...) will suppress logging for coroutine B mid-execution, and coroutine B's logger.enable(...) in its finally block will re-enable logging while coroutine A still expects it to be disabled.
The same applies to redirect_stdout(io.StringIO()) and redirect_stderr(io.StringIO()): these patch the global sys.stdout/sys.stderr, which are shared across all concurrently running coroutines. Output from unrelated coroutines running while these redirects are active will be silently dropped into the StringIO buffer.
A per-handler loguru sink approach or structlog's contextvars-based filtering would be safe for async; the global mutable approach used here is not.
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/tools.py
Line: 537-546
Comment:
**Global logger suppression is not async-safe**
`logger.disable("codebase_rag")` modifies loguru's global state, which is shared across all coroutines. In a concurrent async server, if two `ask_agent` calls are in-flight simultaneously, coroutine A's `logger.disable(...)` will suppress logging for coroutine B mid-execution, and coroutine B's `logger.enable(...)` in its `finally` block will re-enable logging while coroutine A still expects it to be disabled.
The same applies to `redirect_stdout(io.StringIO())` and `redirect_stderr(io.StringIO())`: these patch the global `sys.stdout`/`sys.stderr`, which are shared across all concurrently running coroutines. Output from unrelated coroutines running while these redirects are active will be silently dropped into the `StringIO` buffer.
A per-handler loguru sink approach or structlog's contextvars-based filtering would be safe for async; the global mutable approach used here is not.
How can I resolve this? If you propose a fix, please make it concise.| except Exception: | ||
| # Fail silently without logging or printing error details | ||
| return { | ||
| "output": "There was an error processing your question", | ||
| "error": True, | ||
| } |
There was a problem hiding this comment.
Errors swallowed silently after logging is re-enabled
By the time the outer except Exception: block is reached, the finally clause on line 544 has already re-enabled logging via logger.enable("codebase_rag"). The exception is therefore catchable and loggable, but the block discards it entirely — returning only a generic "There was an error processing your question" string with no trace of what went wrong.
This makes production debugging extremely difficult. The exception should at minimum be logged at logger.error(...) before returning the fallback response:
except Exception as e:
logger.error(f"[MCP] ask_agent failed: {e}", exc_info=True)
return {
"output": "There was an error processing your question",
"error": True,
}Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/tools.py
Line: 547-552
Comment:
**Errors swallowed silently after logging is re-enabled**
By the time the outer `except Exception:` block is reached, the `finally` clause on line 544 has already re-enabled logging via `logger.enable("codebase_rag")`. The exception is therefore catchable and loggable, but the block discards it entirely — returning only a generic `"There was an error processing your question"` string with no trace of what went wrong.
This makes production debugging extremely difficult. The exception should at minimum be logged at `logger.error(...)` before returning the fallback response:
```python
except Exception as e:
logger.error(f"[MCP] ask_agent failed: {e}", exc_info=True)
return {
"output": "There was an error processing your question",
"error": True,
}
```
How can I resolve this? If you propose a fix, please make it concise.| self._function_source_tool = create_get_function_source_tool() | ||
|
|
||
| # Create RAG orchestrator agent (lazy initialization for testing) | ||
| self._rag_agent: Any = None |
There was a problem hiding this comment.
Use Agent type instead of Any
self._rag_agent: Any = None loses all type information. create_rag_orchestrator returns Agent (from pydantic_ai), so the field and the property should be typed accordingly:
| self._rag_agent: Any = None | |
| self._rag_agent: Agent | None = None |
The import from pydantic_ai import Agent should be added at the top of the file, and the rag_agent property signature updated to -> Agent: and the setter to value: Agent | None.
This also applies to the property and setter signatures on lines 267 and 290.
Context Used: Rule from dashboard - ## Technical Requirements
Agentic Framework
- PydanticAI Only: This project uses PydanticAI... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/tools.py
Line: 106
Comment:
**Use `Agent` type instead of `Any`**
`self._rag_agent: Any = None` loses all type information. `create_rag_orchestrator` returns `Agent` (from `pydantic_ai`), so the field and the property should be typed accordingly:
```suggestion
self._rag_agent: Agent | None = None
```
The import `from pydantic_ai import Agent` should be added at the top of the file, and the `rag_agent` property signature updated to `-> Agent:` and the setter to `value: Agent | None`.
This also applies to the property and setter signatures on lines 267 and 290.
**Context Used:** Rule from `dashboard` - ## Technical Requirements
### Agentic Framework
- **PydanticAI Only**: This project uses PydanticAI... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.| import io | ||
| from contextlib import redirect_stderr, redirect_stdout |
There was a problem hiding this comment.
Move imports to module level
import io and from contextlib import redirect_stderr, redirect_stdout are placed inside the function body. Per Python convention (and PEP 8), imports belong at the top of the module. Move both to the top-level import block alongside the other stdlib imports.
| import io | |
| from contextlib import redirect_stderr, redirect_stdout | |
| # Suppress all logging output during agent execution | |
| try: |
The two import lines can be removed from here once they are moved to the module-level import section.
Context Used: Rule from dashboard - ## Technical Requirements
Agentic Framework
- PydanticAI Only: This project uses PydanticAI... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/tools.py
Line: 531-532
Comment:
**Move imports to module level**
`import io` and `from contextlib import redirect_stderr, redirect_stdout` are placed inside the function body. Per Python convention (and PEP 8), imports belong at the top of the module. Move both to the top-level import block alongside the other stdlib imports.
```suggestion
# Suppress all logging output during agent execution
try:
```
The two import lines can be removed from here once they are moved to the module-level import section.
**Context Used:** Rule from `dashboard` - ## Technical Requirements
### Agentic Framework
- **PydanticAI Only**: This project uses PydanticAI... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.| self.document_analyzer = DocumentAnalyzer(project_root=project_root) | ||
|
|
||
| # Create pydantic-ai tools - we'll call the underlying functions directly | ||
| # Use a Console that outputs to stderr to avoid corrupting JSONRPC on stdout |
There was a problem hiding this comment.
Inline comments violate the comment policy
The project's comment policy only permits inline comments that are: (1) top-of-file, (2) prefixed with (H), or (3) type annotations. None of the newly added inline comments qualify.
This applies to every inline comment introduced in this PR:
codebase_rag/mcp/tools.py:84—# Use a Console that outputs to stderr to avoid corrupting JSONRPC on stdoutcodebase_rag/mcp/tools.py:105—# Create RAG orchestrator agent (lazy initialization for testing)codebase_rag/mcp/tools.py:534—# Suppress all logging output during agent executioncodebase_rag/mcp/tools.py:536—# Temporarily redirect stdout and stderr to suppress all outputcodebase_rag/mcp/tools.py:538—# Temporarily disable loguru loggingcodebase_rag/mcp/tools.py:541—# Run the query using the RAG agentcodebase_rag/mcp/tools.py:544—# Re-enable loggingcodebase_rag/mcp/tools.py:547—# Fail silently without logging or printing error details
Remove or prefix all of these with (H) if the intent is genuinely human-authored explanation.
Context Used: Rule from dashboard - ## Technical Requirements
Agentic Framework
- PydanticAI Only: This project uses PydanticAI... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/tools.py
Line: 84
Comment:
**Inline comments violate the comment policy**
The project's comment policy only permits inline comments that are: (1) top-of-file, (2) prefixed with `(H)`, or (3) type annotations. None of the newly added inline comments qualify.
This applies to every inline comment introduced in this PR:
- `codebase_rag/mcp/tools.py:84` — `# Use a Console that outputs to stderr to avoid corrupting JSONRPC on stdout`
- `codebase_rag/mcp/tools.py:105` — `# Create RAG orchestrator agent (lazy initialization for testing)`
- `codebase_rag/mcp/tools.py:534` — `# Suppress all logging output during agent execution`
- `codebase_rag/mcp/tools.py:536` — `# Temporarily redirect stdout and stderr to suppress all output`
- `codebase_rag/mcp/tools.py:538` — `# Temporarily disable loguru logging`
- `codebase_rag/mcp/tools.py:541` — `# Run the query using the RAG agent`
- `codebase_rag/mcp/tools.py:544` — `# Re-enable logging`
- `codebase_rag/mcp/tools.py:547` — `# Fail silently without logging or printing error details`
Remove or prefix all of these with `(H)` if the intent is genuinely human-authored explanation.
**Context Used:** Rule from `dashboard` - ## Technical Requirements
### Agentic Framework
- **PydanticAI Only**: This project uses PydanticAI... ([source](https://app.greptile.com/review/custom-context?memory=d4240b05-b763-467a-a6bf-94f73e8b6859))
How can I resolve this? If you propose a fix, please make it concise.![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extends the MCP tool registry by adding several new tools, including shell_command, document_analyzer, semantic_search, and a new ask_agent tool. However, the current implementation introduces significant security flaws. The ask_agent tool allows a nested LLM agent to execute state-changing shell commands and file modifications without user confirmation, leading to potential privilege escalation. Furthermore, the DocumentAnalyzer and ShellCommander tools are vulnerable to path traversal, allowing arbitrary file access on the host system via absolute paths. Beyond these security concerns, it is recommended to improve error handling in the new ask_agent method by adding server-side logging for exceptions and documenting the rationale for broad exception handling, which is currently missing and would make debugging difficult.
| tools=[ | ||
| self._query_tool, | ||
| self._code_tool, | ||
| self._file_reader_tool, | ||
| self._file_writer_tool, | ||
| self._file_editor_tool, | ||
| self._shell_command_tool, | ||
| self._directory_lister_tool, | ||
| self._document_analyzer_tool, | ||
| self._semantic_search_tool, | ||
| self._function_source_tool, | ||
| ] |
There was a problem hiding this comment.
The ask_agent tool uses a nested RAG orchestrator agent that is configured with powerful, state-changing tools like execute_shell_command, write_file, and replace_code_surgically. While ShellCommander has a confirmation mechanism, ask_agent is non-interactive and suppresses logging. A malicious user can craft a question that tricks the nested agent into executing these tools by autonomously setting user_confirmed=True, effectively bypassing intended security controls without the actual user's oversight. It is recommended to restrict the rag_agent to read-only tools for non-interactive execution.
tools=[
self._query_tool,
self._code_tool,
self._file_reader_tool,
self._directory_lister_tool,
self._document_analyzer_tool,
self._semantic_search_tool,
self._function_source_tool,
]References
- For security-sensitive features like shell command execution by an LLM agent, prioritize fail-safe behavior. Prefer false positives (blocking a safe command) over false negatives (allowing a dangerous one), especially if the fix for the false positive adds complexity.
| self.file_writer = FileWriter(project_root=project_root) | ||
| self.directory_lister = DirectoryLister(project_root=project_root) | ||
| self.shell_commander = ShellCommander(project_root=project_root) | ||
| self.document_analyzer = DocumentAnalyzer(project_root=project_root) |
There was a problem hiding this comment.
The DocumentAnalyzer tool (initialized here) contains a path traversal vulnerability in its analyze method. It explicitly handles absolute paths by copying the target file to a .tmp directory without verifying if the path is within the project root. This allows an attacker to read any file on the system that the process has access to by providing an absolute path.
| self.file_reader = FileReader(project_root=project_root) | ||
| self.file_writer = FileWriter(project_root=project_root) | ||
| self.directory_lister = DirectoryLister(project_root=project_root) | ||
| self.shell_commander = ShellCommander(project_root=project_root) |
There was a problem hiding this comment.
The ShellCommander tool (initialized here) allows for arbitrary file access. While it checks if the command name is in an allowlist (e.g., cat, ls), it does not validate or sanitize the arguments. An attacker can provide absolute paths as arguments to read or list files outside the project root (e.g., execute_shell_command(command="cat /etc/passwd")).
| except Exception: | ||
| # Fail silently without logging or printing error details | ||
| return { | ||
| "output": "There was an error processing your question", | ||
| "error": True, | ||
| } |
There was a problem hiding this comment.
While it's important to return a generic error to the client to avoid leaking implementation details or corrupting the JSON-RPC stream, swallowing the exception without logging it on the server side makes debugging failures nearly impossible. The try...finally block for logger.enable ensures that logging is re-enabled before this except block is reached. Therefore, the exception should be logged here to aid maintainability and troubleshooting. Additionally, if a broad Exception is caught due to unpredictable errors from external libraries, a comment explaining this rationale should be added for maintainability, as per repository guidelines.
except Exception as e:
# Log the error for debugging, but return a generic message to the client
# Catching a broad Exception here is pragmatic due to the wide and unpredictable
# variety of exceptions that can be raised by external libraries.
logger.error(f"[MCP] Error in ask_agent: {e}", exc_info=True)
return {
"output": "There was an error processing your question",
"error": True,
}References
- When handling errors from external libraries that may raise multiple types of exceptions for configuration issues (e.g.,
ValueErrorfor invalid formats andAssertionErrorfor missing keys), catch a tuple of these specific exceptions, such as(ValueError, AssertionError), instead of a genericException. - When interacting with external libraries (like pydantic-ai) that can raise non-standard exceptions (e.g.,
AssertionErrorfor configuration issues), it may be necessary to catch a broaderExceptionor a tuple of specific exceptions (ValueError,AssertionError) to ensure all user-facing errors are handled gracefully. - If catching a broad
Exceptionis a pragmatic choice due to a wide and unpredictable variety of exceptions from external libraries, add a comment explaining the rationale to improve code maintainability.
2 participants
Extend MCPToolsRegistry with ShellCommander, DocumentAnalyzer, semantic search, and get-function-source tools. Add a lazy-initialized ask_agent method that routes a question through the RAG orchestrator and returns structured output. Uses stderr Console to avoid corrupting JSONRPC on stdout.
Dependency graph
Merge in this order: