Conversation
![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 introduces an integration between ms-agent and nanobot via the Model Context Protocol (MCP), exposing capabilities like deep research, web search, LSP code validation, and concurrent-safe file editing. The implementation includes a capability registry, an async task manager for long-running processes, and an MCP server adapter. Feedback focuses on critical safety and security improvements: addressing task-safety issues with global sys.stdout redirection to prevent protocol corruption, fixing a path traversal vulnerability in filesystem tools, ensuring background subprocesses are properly reaped upon cancellation to avoid zombie processes, and adding input validation for line-range operations.
| else: | ||
| _register_cap(server, registry, cap, workspace) | ||
|
|
||
| server.run(transport=args.transport) |
There was a problem hiding this comment.
To safely prevent stray library logs or print() calls from corrupting the MCP stdio JSON-RPC stream, it is recommended to call _protect_stdout() once globally during server startup. This is safer and more efficient than per-request redirection.
| server.run(transport=args.transport) | |
| _protect_stdout() | |
| server.run(transport=args.transport) |
| saved = sys.stdout | ||
| sys.stdout = sys.stderr | ||
| try: | ||
| result = await registry.invoke(cap_name, kw, workspace=workspace) | ||
| finally: | ||
| sys.stdout = saved | ||
| return json.dumps(result, ensure_ascii=False) |
There was a problem hiding this comment.
The per-request redirection of sys.stdout to sys.stderr is not task-safe in an asynchronous environment. If multiple tool calls are processed concurrently, they will interfere with each other's sys.stdout state, potentially leading to race conditions where the protocol stream is corrupted. If _protect_stdout() is called globally at startup, this local redirection is redundant and should be removed.
| saved = sys.stdout | |
| sys.stdout = sys.stderr | |
| try: | |
| result = await registry.invoke(cap_name, kw, workspace=workspace) | |
| finally: | |
| sys.stdout = saved | |
| return json.dumps(result, ensure_ascii=False) | |
| result = await registry.invoke(cap_name, kw, workspace=workspace) | |
| return json.dumps(result, ensure_ascii=False) |
| def _redirect_stdout(): | ||
| """Redirect stdout to stderr while running an in-process LLMAgent. | ||
|
|
||
| MCP stdio transport uses stdout for JSONRPC messages. LLMAgent.step() | ||
| writes streaming content and reasoning to sys.stdout, which would | ||
| corrupt the protocol. Redirecting to stderr keeps the channel clean | ||
| while still allowing the output to appear in server logs. | ||
| """ | ||
| old_stdout = sys.stdout | ||
| sys.stdout = sys.stderr | ||
| try: | ||
| yield | ||
| finally: | ||
| sys.stdout = old_stdout | ||
|
|
There was a problem hiding this comment.
The _redirect_stdout context manager modifies the global sys.stdout state, which is not task-safe in an async environment. Concurrent executions of agents will interfere with each other. It is better to rely on a global redirection at the MCP server level.
# Removed: global sys.stdout redirection is not task-safe.
# Use global redirection in mcp_server.py instead.| def _resolve_path(path: str, workspace: str | None) -> str: | ||
| """Resolve *path* against an optional workspace root.""" | ||
| if os.path.isabs(path): | ||
| return path | ||
| if workspace: | ||
| return os.path.join(workspace, path) | ||
| return os.path.abspath(path) |
There was a problem hiding this comment.
The _resolve_path function is vulnerable to path traversal. It does not verify that the resolved path remains within the intended workspace. An agent could provide a path like ../../etc/passwd to access sensitive files. Additionally, allowing absolute paths (lines 121-122) completely bypasses the workspace restriction.
| def _resolve_path(path: str, workspace: str | None) -> str: | |
| """Resolve *path* against an optional workspace root.""" | |
| if os.path.isabs(path): | |
| return path | |
| if workspace: | |
| return os.path.join(workspace, path) | |
| return os.path.abspath(path) | |
| def _resolve_path(path: str, workspace: str | None) -> str: | |
| """Resolve *path* against an optional workspace root and prevent traversal.""" | |
| if workspace: | |
| abs_workspace = os.path.abspath(workspace) | |
| abs_path = os.path.abspath(os.path.join(abs_workspace, path)) | |
| if not abs_path.startswith(abs_workspace): | |
| raise ValueError(f"Access denied: {path} is outside workspace {workspace}") | |
| return abs_path | |
| return os.path.abspath(path) |
| if task._process and task._process.returncode is None: | ||
| try: | ||
| task._process.kill() | ||
| except ProcessLookupError: | ||
| pass |
There was a problem hiding this comment.
When a task is cancelled, the subprocess is killed using task._process.kill(), but it is never waited upon. This can lead to zombie processes accumulating on the system, as the process remains in the process table until the parent reaps it.
| if task._process and task._process.returncode is None: | |
| try: | |
| task._process.kill() | |
| except ProcessLookupError: | |
| pass | |
| if task._process and task._process.returncode is None: | |
| try: | |
| task._process.kill() | |
| await task._process.wait() | |
| except ProcessLookupError: | |
| pass |
| if end_line is None: | ||
| return {'error': 'end_line is required when start_line > 0'} |
There was a problem hiding this comment.
Validation is missing to ensure that end_line is greater than or equal to start_line. If start_line > end_line, the subsequent slicing logic will result in incorrect file content due to overlapping ranges.
| if end_line is None: | |
| return {'error': 'end_line is required when start_line > 0'} | |
| if end_line is None: | |
| return {'error': 'end_line is required when start_line > 0'} | |
| if end_line < start_line: | |
| return {'error': f'end_line ({end_line}) must be >= start_line ({start_line})'} |
2 participants
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.