feat(mcp/server): improve MCP server logging and suppress output during tool execution#400
feat(mcp/server): improve MCP server logging and suppress output during tool execution#400
Conversation
…ng tool execution
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 significantly enhances the MCP server's logging and output management. It transitions the logging system from direct Highlights
Changelog
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
|
![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 refactors the MCP server's logging to be file-based and optional, and suppresses stdout/stderr during tool execution, which are good improvements for an LLM agent environment. However, the current implementation uses global state redirection (redirect_stdout and logger.disable) in an asynchronous context, creating a race condition that can corrupt the server's communication channel during concurrent tool executions, leading to a denial of service. Additionally, the implementation introduces critical error handling issues where exceptions are caught and silenced without being logged, making debugging difficult. There are also opportunities to simplify logging configuration logic and address a violation of a project rule regarding docstrings.
| except Exception: | ||
| # Fail silently without logging or printing error details | ||
| return [ | ||
| TextContent( | ||
| type="text", text="Error: There was an error executing the tool" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Catching all exceptions and failing silently without logging is a dangerous pattern that can hide critical bugs and make debugging extremely difficult. While the goal is to prevent error details from reaching the LLM, they absolutely should be logged to the configured file (if logging is enabled). Using logger.exception() will handle this correctly, as it will only log if the logger is enabled and will include valuable traceback information.
except Exception:
# Log the exception if logging is enabled, but return a generic error to the client.
logger.exception(f"[GraphCode MCP] Error executing tool '{name}'")
return [
TextContent(
type="text", text="Error: There was an error executing the tool"
)
]| error_msg = "Unknown tool" | ||
| return [TextContent(type="text", text=f"Error: {error_msg}")] |
There was a problem hiding this comment.
While returning a generic error message to the LLM is reasonable to save tokens, removing the log for an unknown tool makes debugging difficult. Please log this error. The log will be correctly routed to a file or disabled based on the setup_logging configuration.
logger.error(f"[GraphCode MCP] Unknown tool: {name}")
return [TextContent(type="text", text="Error: Unknown tool")]| with redirect_stdout(io.StringIO()), redirect_stderr(io.StringIO()): | ||
| logger.disable("codebase_rag") | ||
| try: | ||
| # Call handler with unpacked arguments | ||
| result = await handler(**arguments) | ||
|
|
||
| # Format result based on output type | ||
| if returns_json: | ||
| result_text = json.dumps(result, indent=2) | ||
| else: | ||
| result_text = str(result) | ||
|
|
||
| return [TextContent(type="text", text=result_text)] | ||
| finally: | ||
| logger.enable("codebase_rag") |
There was a problem hiding this comment.
The use of contextlib.redirect_stdout and logger.disable within an asynchronous function creates a race condition that can lead to a Denial of Service (DoS). In Python, sys.stdout and the loguru logger's enabled/disabled state are global. Since call_tool is an async function, multiple tool calls can execute concurrently. If one tool call redirects stdout or disables logging, it affects all other concurrent tasks, including the MCP server's own transport mechanism which relies on stdout for JSON-RPC communication. This can result in the corruption of the transport protocol, causing the server to crash or become unresponsive when multiple tools are invoked simultaneously.
| """Configure logging for MCP stdio transport. | ||
|
|
||
| By default, logging is disabled to prevent token waste in LLM context. | ||
| Can be enabled via environment variable MCP_ENABLE_LOGGING=1 for debugging. | ||
|
|
||
| When enabled, logs are written to a file to avoid polluting STDIO transport. | ||
| The log file path can be configured via MCP_LOG_FILE environment variable. | ||
|
|
||
| Args: | ||
| enable_logging: Whether to enable logging output. Defaults to False. | ||
| Can also be controlled via MCP_ENABLE_LOGGING environment variable. | ||
| """ |
There was a problem hiding this comment.
According to the project's general rules, docstrings are not allowed. Please convert this docstring into a regular block comment to adhere to the project's standards.
# Configure logging for MCP stdio transport.
#
# By default, logging is disabled to prevent token waste in LLM context.
# Can be enabled via environment variable MCP_ENABLE_LOGGING=1 for debugging.
#
# When enabled, logs are written to a file to avoid polluting STDIO transport.
# The log file path can be configured via MCP_LOG_FILE environment variable.
#
# Args:
# enable_logging: Whether to enable logging output. Defaults to False.
# Can also be controlled via MCP_ENABLE_LOGGING environment variable.References
- Docstrings are not allowed in this project, as enforced by a pre-commit hook.
| log_file = os.environ.get("MCP_LOG_FILE") | ||
| if not log_file: | ||
| # Use ~/.cache/code-graph-rag/mcp.log as default | ||
| cache_dir = Path.home() / ".cache" / "code-graph-rag" | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| log_file = str(cache_dir / "mcp.log") | ||
|
|
||
| # Ensure log file directory exists | ||
| log_path = Path(log_file) | ||
| log_path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
The logic for determining the log file path and creating its parent directory can be simplified to avoid redundant mkdir calls and improve clarity. You can determine the Path object for the log file first, and then call mkdir once on its parent directory.
log_file_path_str = os.environ.get("MCP_LOG_FILE")
if log_file_path_str:
log_file = Path(log_file_path_str)
else:
# Use ~/.cache/code-graph-rag/mcp.log as default
cache_dir = Path.home() / ".cache" / "code-graph-rag"
log_file = cache_dir / "mcp.log"
# Ensure log file directory exists
log_file.parent.mkdir(parents=True, exist_ok=True)
Greptile SummaryRefactors MCP server logging to write to a rotating file instead of stderr, with opt-in via Critical Issue:
Style Violations (per rule d4240b05-b763-467a-a6bf-94f73e8b6859):
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: 1183bb1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| logger.disable("codebase_rag") | ||
| try: | ||
| # Call handler with unpacked arguments | ||
| result = await handler(**arguments) | ||
|
|
||
| # Format result based on output type | ||
| if returns_json: | ||
| result_text = json.dumps(result, indent=2) | ||
| else: | ||
| result_text = str(result) | ||
|
|
||
| return [TextContent(type="text", text=result_text)] | ||
| finally: | ||
| logger.enable("codebase_rag") |
There was a problem hiding this comment.
logger.enable() in finally re-enables logging globally, even when setup_logging() disabled it. After first tool call, all logs will appear despite MCP_ENABLE_LOGGING not being set.
Check if logging was enabled before disabling, and only restore previous state:
# Track if logging was enabled
logging_was_enabled = not logger._core.enabled.get("codebase_rag", False)
if logging_was_enabled:
logger.disable("codebase_rag")
try:
result = await handler(**arguments)
# ...
finally:
if logging_was_enabled:
logger.enable("codebase_rag")Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 202-215
Comment:
`logger.enable()` in finally re-enables logging globally, even when `setup_logging()` disabled it. After first tool call, all logs will appear despite `MCP_ENABLE_LOGGING` not being set.
Check if logging was enabled before disabling, and only restore previous state:
```python
# Track if logging was enabled
logging_was_enabled = not logger._core.enabled.get("codebase_rag", False)
if logging_was_enabled:
logger.disable("codebase_rag")
try:
result = await handler(**arguments)
# ...
finally:
if logging_was_enabled:
logger.enable("codebase_rag")
```
How can I resolve this? If you propose a fix, please make it concise.| cache_dir = Path.home() / ".cache" / "code-graph-rag" | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| log_file = str(cache_dir / "mcp.log") | ||
|
|
||
| # Ensure log file directory exists | ||
| log_path = Path(log_file) | ||
| log_path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Directory created twice in default path case - cache_dir.mkdir() at 51, then log_path.parent.mkdir() at 56 creates same directory.
| cache_dir = Path.home() / ".cache" / "code-graph-rag" | |
| cache_dir.mkdir(parents=True, exist_ok=True) | |
| log_file = str(cache_dir / "mcp.log") | |
| # Ensure log file directory exists | |
| log_path = Path(log_file) | |
| log_path.parent.mkdir(parents=True, exist_ok=True) | |
| log_file = os.environ.get("MCP_LOG_FILE") | |
| if not log_file: | |
| # Use ~/.cache/code-graph-rag/mcp.log as default | |
| cache_dir = Path.home() / ".cache" / "code-graph-rag" | |
| log_file = str(cache_dir / "mcp.log") | |
| # Ensure log file directory exists (handles both env and default cases) | |
| Path(log_file).parent.mkdir(parents=True, exist_ok=True) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 50-56
Comment:
Directory created twice in default path case - `cache_dir.mkdir()` at 51, then `log_path.parent.mkdir()` at 56 creates same directory.
```suggestion
log_file = os.environ.get("MCP_LOG_FILE")
if not log_file:
# Use ~/.cache/code-graph-rag/mcp.log as default
cache_dir = Path.home() / ".cache" / "code-graph-rag"
log_file = str(cache_dir / "mcp.log")
# Ensure log file directory exists (handles both env and default cases)
Path(log_file).parent.mkdir(parents=True, exist_ok=True)
```
How can I resolve this? If you propose a fix, please make it concise.| env_enable = os.environ.get("MCP_ENABLE_LOGGING", "").lower() in ( | ||
| "1", | ||
| "true", | ||
| "yes", | ||
| ) | ||
| should_enable = enable_logging or env_enable | ||
|
|
||
| if should_enable: | ||
| # Get log file path from environment or use default | ||
| log_file = os.environ.get("MCP_LOG_FILE") | ||
| if not log_file: | ||
| # Use ~/.cache/code-graph-rag/mcp.log as default | ||
| cache_dir = Path.home() / ".cache" / "code-graph-rag" | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| log_file = str(cache_dir / "mcp.log") | ||
|
|
||
| # Ensure log file directory exists | ||
| log_path = Path(log_file) | ||
| log_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Add file handler - logs go to file, not STDERR/STDOUT | ||
| logger.add( | ||
| log_file, | ||
| level="INFO", | ||
| format="{time:YYYY-MM-DD HH:mm:ss} | {level: <8} | {message}", | ||
| colorize=False, # Disable ANSI color codes | ||
| rotation="10 MB", # Rotate when file reaches 10MB | ||
| retention="7 days", # Keep logs for 7 days | ||
| ) | ||
| else: | ||
| # Disable all logging by default for MCP mode | ||
| logger.disable("codebase_rag") |
There was a problem hiding this comment.
Hardcoded strings violate coding standards. Per rule d4240b05, non-config/constants/logs files should have almost no string literals. Move to codebase_rag/mcp/constants.py:
# constants.py
from enum import StrEnum
ENV_MCP_ENABLE_LOGGING = "MCP_ENABLE_LOGGING"
ENV_MCP_LOG_FILE = "MCP_LOG_FILE"
ENABLE_VALUES = ("1", "true", "yes")
DEFAULT_CACHE_DIR = ".cache"
DEFAULT_PROJECT_NAME = "code-graph-rag"
DEFAULT_LOG_FILE = "mcp.log"
LOG_FORMAT = "{time:YYYY-MM-DD HH:mm:ss} | {level: <8} | {message}"
LOG_ROTATION = "10 MB"
LOG_RETENTION = "7 days"
LOGGER_NAME = "codebase_rag"Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 38-69
Comment:
Hardcoded strings violate coding standards. Per rule d4240b05, non-config/constants/logs files should have almost no string literals. Move to `codebase_rag/mcp/constants.py`:
```python
# constants.py
from enum import StrEnum
ENV_MCP_ENABLE_LOGGING = "MCP_ENABLE_LOGGING"
ENV_MCP_LOG_FILE = "MCP_LOG_FILE"
ENABLE_VALUES = ("1", "true", "yes")
DEFAULT_CACHE_DIR = ".cache"
DEFAULT_PROJECT_NAME = "code-graph-rag"
DEFAULT_LOG_FILE = "mcp.log"
LOG_FORMAT = "{time:YYYY-MM-DD HH:mm:ss} | {level: <8} | {message}"
LOG_ROTATION = "10 MB"
LOG_RETENTION = "7 days"
LOGGER_NAME = "codebase_rag"
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import io | ||
| from contextlib import redirect_stderr, redirect_stdout |
There was a problem hiding this comment.
Imports inside function. Move to module top per Python conventions and project code organization standards.
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 188-189
Comment:
Imports inside function. Move to module top per Python conventions and project code organization standards.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| colorize=False, # Disable ANSI color codes | ||
| rotation="10 MB", # Rotate when file reaches 10MB | ||
| retention="7 days", # Keep logs for 7 days |
There was a problem hiding this comment.
Inline comments without (H) marker violate comment policy. Either prefix with (H) or remove (parameters are self-documenting).
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 63-65
Comment:
Inline comments without `(H)` marker violate comment policy. Either prefix with `(H)` or remove (parameters are self-documenting).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| error_msg = "Unknown tool" | ||
| return [TextContent(type="text", text=f"Error: {error_msg}")] | ||
|
|
||
| handler, returns_json = handler_info | ||
|
|
||
| # Call handler with unpacked arguments | ||
| result = await handler(**arguments) | ||
|
|
||
| # Format result based on output type | ||
| if returns_json: | ||
| result_text = json.dumps(result, indent=2) | ||
| else: | ||
| result_text = str(result) | ||
|
|
||
| return [TextContent(type="text", text=result_text)] | ||
|
|
||
| except Exception as e: | ||
| error_msg = f"Error executing tool '{name}': {str(e)}" | ||
| logger.error(f"[GraphCode MCP] {error_msg}", exc_info=True) | ||
| return [TextContent(type="text", text=f"Error: {error_msg}")] | ||
| # Suppress all logging output during tool execution | ||
| with redirect_stdout(io.StringIO()), redirect_stderr(io.StringIO()): | ||
| logger.disable("codebase_rag") | ||
| try: | ||
| # Call handler with unpacked arguments | ||
| result = await handler(**arguments) | ||
|
|
||
| # Format result based on output type | ||
| if returns_json: | ||
| result_text = json.dumps(result, indent=2) | ||
| else: | ||
| result_text = str(result) | ||
|
|
||
| return [TextContent(type="text", text=result_text)] | ||
| finally: | ||
| logger.enable("codebase_rag") | ||
|
|
||
| except Exception: | ||
| # Fail silently without logging or printing error details | ||
| return [ | ||
| TextContent( | ||
| type="text", text="Error: There was an error executing the tool" |
There was a problem hiding this comment.
Error messages are hardcoded strings. Move to tool_errors.py per coding standards:
# tool_errors.py
from enum import Enum
class ToolError(str, Enum):
UNKNOWN_TOOL = "Unknown tool"
EXECUTION_FAILED = "There was an error executing the tool"
def __call__(self, **kwargs) -> str:
return self.value.format(**kwargs) if kwargs else self.valuePrompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/mcp/server.py
Line: 195-221
Comment:
Error messages are hardcoded strings. Move to `tool_errors.py` per coding standards:
```python
# tool_errors.py
from enum import Enum
class ToolError(str, Enum):
UNKNOWN_TOOL = "Unknown tool"
EXECUTION_FAILED = "There was an error executing the tool"
def __call__(self, **kwargs) -> str:
return self.value.format(**kwargs) if kwargs else self.value
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Refactor setup_logging() to write logs to a rotating file instead of stderr, with opt-in via MCP_ENABLE_LOGGING env var. Suppress stdout/stderr and loguru output during tool handler execution to prevent corrupting STDIO transport and wasting LLM context tokens.
Dependency graph
Merge in this order: