feat: add MCP tool orchestration and server config support#2348
Open
universam1 wants to merge 1 commit intoThe-PR-Agent:mainfrom
Open
feat: add MCP tool orchestration and server config support#2348universam1 wants to merge 1 commit intoThe-PR-Agent:mainfrom
universam1 wants to merge 1 commit intoThe-PR-Agent:mainfrom
Conversation
Contributor
Review Summary by QodoAdd MCP tool orchestration and server configuration support
WalkthroughsDescription• Add MCP (Model Context Protocol) runtime with stdio and HTTP client support • Implement structured tool-calling orchestration in base AI handler • Load MCP server configuration from JSON/JSONC files with schema normalization • Integrate MCP tools into /ask, /review, and /improve commands • Expose MCP runtime status in /config output with comprehensive tests Diagramflowchart LR
ConfigLoader["Config Loader<br/>JSONC Parsing"] -->|loads| MCPConfig["MCP Server Config<br/>servers/mcpServers"]
MCPConfig -->|initializes| MCPRuntime["MCP Runtime<br/>stdio/HTTP clients"]
MCPRuntime -->|discovers| ToolCatalog["Tool Catalog<br/>schemas"]
ToolCatalog -->|passed to| ToolOrch["Tool Orchestration<br/>chat_completion_with_tools"]
ToolOrch -->|executes| Commands["Commands<br/>ask/review/improve"]
Commands -->|status in| ConfigOutput["/config output"]
File Changes1. pr_agent/algo/ai_handlers/base_ai_handler.py
|
Contributor
Code Review by Qodo
1. Unused io import
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
| raise MCPRuntimeError( | ||
| f"MCP server '{server_name}' uses unsupported transport type '{server_type}'" | ||
| ) No newline at end of file |
Contributor
There was a problem hiding this comment.
1. Files missing final newline 📘 Rule violation ⚙ Maintainability
Several newly added files end without a trailing newline, which commonly fails pre-commit hooks (end-of-file-fixer) and can break CI. Add a final newline to each affected file.
Agent Prompt
## Issue description
Multiple newly added files are missing a final newline, which can cause pre-commit/CI failures.
## Issue Context
The diff marks these files with `No newline at end of file`.
## Fix Focus Areas
- pr_agent/mcp/__init__.py[15-15]
- pr_agent/mcp/integration.py[62-62]
- pr_agent/mcp/runtime.py[499-499]
- tests/unittest/test_mcp_runtime.py[127-127]
- tests/unittest/test_mcp_tool_discovery.py[38-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+24
to
+62
| runtime = MCPRuntime() | ||
| if not runtime.enabled: | ||
| return await ai_handler.chat_completion( | ||
| model=model, | ||
| system=system, | ||
| user=user, | ||
| temperature=temperature, | ||
| img_path=img_path, | ||
| ) | ||
|
|
||
| max_tools = _get_tool_budget("MCP.MAX_TOOL_CATALOG_TOOLS", 12) | ||
| max_schema_chars = _get_tool_budget("MCP.MAX_TOOL_CATALOG_SCHEMA_CHARS", 12000) | ||
|
|
||
| tools = runtime.build_tool_schemas( | ||
| max_tools=max_tools, | ||
| max_schema_chars=max_schema_chars, | ||
| include_server_prefix=True, | ||
| ) | ||
| if not tools: | ||
| return await ai_handler.chat_completion( | ||
| model=model, | ||
| system=system, | ||
| user=user, | ||
| temperature=temperature, | ||
| img_path=img_path, | ||
| ) | ||
|
|
||
| if command_name: | ||
| system = f"{system}\n\nCommand context: {command_name}" | ||
|
|
||
| return await ai_handler.chat_completion_with_tools( | ||
| model=model, | ||
| system=system, | ||
| user=user, | ||
| tools=tools, | ||
| tool_executor=runtime.create_tool_executor(), | ||
| temperature=temperature, | ||
| img_path=img_path, | ||
| ) No newline at end of file |
Contributor
There was a problem hiding this comment.
2. Mcp clients never closed 🐞 Bug ☼ Reliability
maybe_chat_completion_with_mcp creates a new MCPRuntime, connects to servers during tool discovery/calls, and returns without disconnecting; this leaks requests sessions and leaves stdio subprocesses running across requests. In a long-running server, repeated /ask,/review,/improve calls can exhaust file descriptors/process slots and degrade performance or crash the service.
Agent Prompt
### Issue description
`maybe_chat_completion_with_mcp()` creates an `MCPRuntime()` and implicitly connects servers during tool discovery/execution, but it never calls `disconnect_all()`. This leaks `requests.Session` objects and leaves stdio subprocesses running.
### Issue Context
- `build_tool_schemas()` -> `list_all_tools()` -> `list_server_tools()` can connect servers.
- `MCPRuntime` provides `disconnect_all()` to close sessions/terminate processes.
### Fix Focus Areas
- pr_agent/mcp/integration.py[15-62]
- pr_agent/mcp/runtime.py[344-375]
### Suggested fix
Wrap the MCP-enabled execution path in `try/finally` and ensure `runtime.disconnect_all()` runs on *all* return paths (disabled, no tools, successful tool loop, exceptions). If you want to keep connections for reuse, refactor to a shared runtime with explicit lifecycle management (startup/shutdown) instead of per-request instantiation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+195
to
+224
| def _read_response(self, request_id: int) -> dict[str, Any]: | ||
| while True: | ||
| message = self._read_message() | ||
| if message.get("id") == request_id: | ||
| return message | ||
|
|
||
| def _read_message(self) -> dict[str, Any]: | ||
| if not self.process or not self.process.stdout: | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' is not connected") | ||
|
|
||
| headers: dict[str, str] = {} | ||
| while True: | ||
| line = self.process.stdout.readline() | ||
| if line == b"": | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' closed stdout unexpectedly") | ||
| if line in (b"\r\n", b"\n"): | ||
| break | ||
| key, _, value = line.decode("utf-8").partition(":") | ||
| headers[key.strip().lower()] = value.strip() | ||
|
|
||
| content_length_value = headers.get("content-length") | ||
| if not content_length_value: | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' response missing Content-Length") | ||
|
|
||
| content_length = int(content_length_value) | ||
| body = self.process.stdout.read(content_length) | ||
| if not body: | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' returned an empty response body") | ||
|
|
||
| return json.loads(body.decode("utf-8")) |
Contributor
There was a problem hiding this comment.
3. Stdio tool calls can hang 🐞 Bug ☼ Reliability
MCPStdioClient performs fully blocking reads (readline/read) and loops until a matching request id is seen, with no timeout or process-liveness checks. If a server stalls, emits partial frames, or never responds, PR-Agent can hang indefinitely while building the tool catalog or executing a tool call.
Agent Prompt
### Issue description
`MCPStdioClient` can block forever waiting for a response frame (`stdout.readline()` / `stdout.read()`), causing PR-Agent requests to hang indefinitely.
### Issue Context
Tool discovery (`tools/list`) happens before model calls, so a hang here can break /ask,/review,/improve even before the LLM is invoked.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[169-225]
### Suggested fix
- Add a configurable timeout for stdio request/response (e.g., default 30s, overridable via server config).
- Implement non-blocking/timeout-aware IO (e.g., `selectors` on `self.process.stdout`), and raise `MCPRuntimeError` on timeout.
- In the wait loop, periodically check `self.process.poll()` and fail fast if the process exited.
- Consider handling notifications (messages without `id`) explicitly so they don’t starve the request loop.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 8008c02 |
Comment on lines
+231
to
+255
| def _read_message(self) -> dict[str, Any]: | ||
| if not self.process or not self.process.stdout: | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' is not connected") | ||
|
|
||
| headers: dict[str, str] = {} | ||
| while True: | ||
| line = self.process.stdout.readline() | ||
| if line == b"": | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' closed stdout unexpectedly") | ||
| if line in (b"\r\n", b"\n"): | ||
| break | ||
| key, _, value = line.decode("utf-8").partition(":") | ||
| headers[key.strip().lower()] = value.strip() | ||
|
|
||
| content_length_value = headers.get("content-length") | ||
| if not content_length_value: | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' response missing Content-Length") | ||
|
|
||
| content_length = int(content_length_value) | ||
| body = self.process.stdout.read(content_length) | ||
| if not body: | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' returned an empty response body") | ||
|
|
||
| return json.loads(body.decode("utf-8")) | ||
|
|
Contributor
There was a problem hiding this comment.
1. Stdio json errors uncaught 🐞 Bug ☼ Reliability
MCPStdioClient._read_message() can raise JSONDecodeError/ValueError when parsing a server response, but MCPRuntime.list_all_tools() only catches MCPRuntimeError/OSError, so a malformed stdio response can crash MCP tool discovery and break MCP-enabled /ask,/review,/improve flows.
Agent Prompt
### Issue description
`MCPStdioClient._read_message()` can raise `json.JSONDecodeError` (and `ValueError` from header parsing) that are not wrapped as `MCPRuntimeError`. Since `MCPRuntime.list_all_tools()` only catches `(MCPRuntimeError, OSError)`, these exceptions can bubble out and crash MCP-enabled commands.
### Issue Context
The runtime tries to be resilient in `list_all_tools()` by catching per-server failures, but that only works if client failures are surfaced as `MCPRuntimeError` (or are explicitly caught).
### Fix Focus Areas
- pr_agent/mcp/runtime.py[231-255]
- pr_agent/mcp/runtime.py[413-423]
### Implementation notes
- In `_read_message()`, wrap `int(content_length_value)` and `json.loads(...)` in `try/except` and raise `MCPRuntimeError` with context (server name, operation) on failure.
- Alternatively (or additionally), broaden `list_all_tools()` to catch `Exception` (or at least `ValueError`) so one misbehaving server can’t break tool discovery.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 7fdc2d8 |
Comment on lines
+95
to
+107
| if remaining_turns <= 0: | ||
| raise ValueError("MCP tool orchestration exceeded the configured turn budget") | ||
|
|
||
| tool_name = str(parsed_response.get("tool", "")).strip() | ||
| arguments = parsed_response.get("arguments") or {} | ||
| if not tool_name: | ||
| raise ValueError("MCP tool orchestration returned an empty tool name") | ||
| if not isinstance(arguments, dict): | ||
| raise ValueError("MCP tool orchestration arguments must be a JSON object") | ||
|
|
||
| tool_result = tool_executor(tool_name, arguments) | ||
| if inspect.isawaitable(tool_result): | ||
| tool_result = await tool_result |
Contributor
There was a problem hiding this comment.
1. Tool loop raises uncaught valueerror 📘 Rule violation ☼ Reliability
chat_completion_with_tools() raises ValueError for common model/tool-loop failure modes (turn budget, empty tool name, non-dict arguments) and does not catch exceptions from tool_executor. This can crash /ask, /review, or /improve instead of failing gracefully.
Agent Prompt
## Issue description
`BaseAiHandler.chat_completion_with_tools()` raises uncaught exceptions (e.g., turn budget exceeded, invalid tool call shape) and does not handle `tool_executor` failures, which can terminate the command instead of returning a safe fallback.
## Issue Context
This method is used as part of MCP tool orchestration and can be triggered by imperfect model outputs or transient tool failures.
## Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[84-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+451
to
+456
| def create_tool_executor(self): | ||
| async def executor(tool_name: str, arguments: Optional[dict[str, Any]] = None): | ||
| server_name, server_tool_name = self._split_tool_name(tool_name) | ||
| return self.call_tool(server_name, server_tool_name, arguments) | ||
|
|
||
| return executor |
Contributor
There was a problem hiding this comment.
2. Blocking mcp tool calls 🐞 Bug ➹ Performance
MCP tool execution performs synchronous HTTP/subprocess I/O inside an async tool loop, which can block the event loop and stall concurrent /ask,/review,/improve requests under load. This occurs because the async tool executor calls a synchronous call path that uses requests.Session.post().
Agent Prompt
### Issue description
MCP tool calls run blocking I/O (requests + subprocess) on the async execution path, which can block the event loop.
### Issue Context
`BaseAiHandler.chat_completion_with_tools()` awaits `tool_executor()`. `MCPRuntime.create_tool_executor()` returns an `async def` that calls the synchronous `call_tool()`, and HTTP tool calls use `requests.Session.post()`.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[451-470]
- pr_agent/mcp/runtime.py[313-346]
- pr_agent/mcp/runtime.py[458-470]
### Implementation direction
- Option A (quick, minimal): wrap synchronous tool execution in `asyncio.to_thread(...)` inside the executor (and similarly for tool discovery if needed).
- Option B (better): replace `requests` with an async HTTP client (e.g., `httpx.AsyncClient`) and consider an asyncio-native stdio transport, so tool listing/calls don’t block the loop.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 7fdc2d8 |
Comment on lines
+88
to
+91
| def _prepare_mcp_status_block(self) -> str: | ||
| status = MCPRuntime().get_status() | ||
| if not status["enabled"] and not status["configured_servers"]: | ||
| return "" |
Contributor
There was a problem hiding this comment.
1. /config mcp status may crash 🐞 Bug ☼ Reliability
PRConfig._prepare_mcp_status_block() calls MCPRuntime().get_status() without handling MCPRuntimeError, so a malformed MCP.SERVERS value can cause the /config command to fail. This makes it harder to debug config problems because /config itself becomes unavailable.
Agent Prompt
### Issue description
`/config` can crash if MCP settings are malformed because MCPRuntime construction errors aren’t handled.
### Issue Context
Users often run `/config` specifically to debug configuration problems; it should degrade gracefully.
### Fix Focus Areas
- pr_agent/tools/pr_config.py[88-100]
- pr_agent/mcp/runtime.py[350-357]
### Suggested fix approach
- Wrap `MCPRuntime().get_status()` in try/except for `Exception` or `MCPRuntimeError`.
- On failure, emit a small status block like:
- `mcp.enabled = unknown`
- `mcp.error = "..."`
- Ensure the rest of the config output still renders.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 75a838c |
Comment on lines
+192
to
+197
| def _read_response(self, request_id: int) -> dict[str, Any]: | ||
| while True: | ||
| message = self._read_message_with_timeout() | ||
| if message.get("id") == request_id: | ||
| return message | ||
|
|
Contributor
There was a problem hiding this comment.
1. No response type-check before .get() 📘 Rule violation ☼ Reliability
MCP client code assumes JSON-RPC responses are dicts and calls .get()/membership checks without validating the decoded JSON shape, which can raise runtime exceptions on malformed or unexpected responses.
Agent Prompt
## Issue description
JSON decoded from MCP servers can be non-dict (or invalid shape). The code currently assumes a dict and calls `.get()` / checks keys, which can crash.
## Issue Context
This is external input from MCP servers (stdio subprocess or HTTP endpoint). Defensive validation should happen immediately after `json.loads()` / `response.json()`.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[192-197]
- pr_agent/mcp/runtime.py[231-255]
- pr_agent/mcp/runtime.py[332-347]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+61
to
+71
| tool_catalog_text = json.dumps(tools, indent=2, sort_keys=True) | ||
| structured_system = ( | ||
| f"{system}\n\n" | ||
| f"Available MCP tools (JSON schema):\n{tool_catalog_text}\n\n" | ||
| "Always inspect the available tools first and use them before responding whenever they can help answer the user's request.\n" | ||
| "When you need a tool, respond with ONLY a JSON object exactly in this shape:\n" | ||
| '{"type":"tool_call","tool":"server.tool","arguments":{...}}\n' | ||
| "Do not include a final answer in the same message as a tool call.\n" | ||
| "When you are finished, respond with ONLY a JSON object exactly in this shape:\n" | ||
| '{"type":"final","content":"..."}\n' | ||
| "Do not wrap the JSON in markdown fences." |
Contributor
There was a problem hiding this comment.
2. Single quotes and long lines 📘 Rule violation ⚙ Maintainability
New Python code introduces single-quoted string literals and very long inline JSON strings that are likely to violate the repository’s Ruff style expectations (double quotes, 120-char lines).
Agent Prompt
## Issue description
Some newly added Python strings likely violate enforced formatting: single quotes where the project prefers double quotes and long inline JSON strings that likely exceed 120 characters.
## Issue Context
Ruff/pre-commit commonly enforces quote style and line length; keeping long JSON examples as concatenated strings or multiline literals improves compliance.
## Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[61-71]
- tests/unittest/test_ai_handler_tool_orchestration.py[75-81]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+89
to
+112
| def connect(self): | ||
| command = self.config.get("command") | ||
| if not command: | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' is missing 'command'") | ||
|
|
||
| args = self.config.get("args") or [] | ||
| if not isinstance(args, list): | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' args must be a list") | ||
|
|
||
| env = os.environ.copy() | ||
| server_env = self.config.get("env") or {} | ||
| if not isinstance(server_env, dict): | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' env must be an object") | ||
| env.update({str(k): str(v) for k, v in server_env.items()}) | ||
|
|
||
| cwd = self.config.get("cwd") | ||
| self.process = subprocess.Popen( | ||
| [command, *args], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, | ||
| env=env, | ||
| cwd=cwd, | ||
| ) |
Contributor
There was a problem hiding this comment.
3. Untrusted mcp command execution 🐞 Bug ⛨ Security
Repository-local pyproject.toml is loaded into settings and can influence MCP enablement/config, and MCP stdio servers are executed via subprocess.Popen from that configuration. This enables untrusted PR content to drive arbitrary command execution (stdio) or outbound requests (HTTP) on the PR-Agent host when MCP is enabled.
Agent Prompt
### Issue description
Untrusted repository configuration (loaded from `pyproject.toml`) can influence MCP settings, and MCP stdio servers are launched via `subprocess.Popen`, creating an RCE/SSRF risk when running PR-Agent on untrusted PR content.
### Issue Context
`config_loader` loads `pyproject.toml` from the repo being reviewed into settings, and MCP runtime/server config is driven from those settings. MCP stdio transport executes OS commands from the server definitions.
### Fix Focus Areas
- pr_agent/config_loader.py[143-149]
- pr_agent/config_loader.py[176-192]
- pr_agent/config_loader.py[195-226]
- pr_agent/mcp/runtime.py[349-395]
- pr_agent/mcp/runtime.py[82-127]
### What to change
- Ensure MCP configuration (enabled flag, config path, and servers) can only come from trusted server-side sources (e.g., environment variables and/or a fixed server config file), not from repository-loaded config.
- Consider disabling `stdio` transport by default (or require an explicit server-side allowlist) in hosted/untrusted modes.
- If you keep repo-level configuration, add a hard trust gate (e.g., only allow MCP when the repo/PR author is trusted) before loading/connecting any MCP servers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit fda158e |
Comment on lines
+134
to
+147
| def list_tools(self) -> list[MCPToolDefinition]: | ||
| result = self._send_request("tools/list", {}) | ||
| tools = result.get("tools") or [] | ||
| parsed: list[MCPToolDefinition] = [] | ||
| for tool in tools: | ||
| parsed.append( | ||
| MCPToolDefinition( | ||
| server_name=self.server_name, | ||
| name=tool.get("name", ""), | ||
| description=tool.get("description", ""), | ||
| input_schema=tool.get("inputSchema", {}), | ||
| ) | ||
| ) | ||
| return parsed |
Contributor
There was a problem hiding this comment.
1. list_tools() assumes dict tools 📘 Rule violation ☼ Reliability
MCP client list_tools() implementations assume each tool entry is a dict and call tool.get(...) without validating shape, which can raise AttributeError on malformed/unexpected server responses. This violates defensive validation for external inputs and can crash tool discovery at runtime.
Agent Prompt
## Issue description
MCP tool discovery assumes each `tools` element is a dict and calls `.get()` directly, which can crash on unexpected server responses.
## Issue Context
MCP server responses are external/untrusted input; code must defensively validate types before calling methods.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[134-147]
- pr_agent/mcp/runtime.py[289-302]
- pr_agent/mcp/runtime.py[396-409]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+75
to
+81
| handler = FakeToolHandler( | ||
| [ | ||
| 'Ask❓\n\nwhat are the latest AWS services released\nAnswer:\n' | ||
| '{"type":"tool_call","tool":"AWS Knowledge.aws___read_documentation","arguments":{"requests":[{"url":"https://aws.amazon.com/about-aws/whats-new/","max_length":8000}]}}\n' | ||
| '{"type":"final","content":"should be ignored in the first turn"}', | ||
| '{"type": "final", "content": "done"}', | ||
| ] |
Contributor
There was a problem hiding this comment.
2. Ruff style violations in tests 📘 Rule violation ⚙ Maintainability
New test code introduces Ruff/formatting violations: very long lines and inconsistent quoting, plus several new files are missing a trailing newline. These can fail repository lint/format tooling and CI checks.
Agent Prompt
## Issue description
New tests/files likely violate Ruff/format tooling (long lines, quote style) and several new files are missing a trailing newline.
## Issue Context
Repo compliance requires keeping code compatible with Ruff/isort/format and common whitespace hooks.
## Fix Focus Areas
- tests/unittest/test_ai_handler_tool_orchestration.py[75-81]
- pr_agent/mcp/__init__.py[1-15]
- pr_agent/mcp/integration.py[1-65]
- pr_agent/mcp/runtime.py[1-708]
- tests/unittest/test_mcp_tool_discovery.py[1-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 74b69d1 |
Comment on lines
+189
to
+192
| except Exception as exc: | ||
| logger.error(f"Failed to load MCP server configuration from {config_path}: {exc}") | ||
| if get_settings().get("MCP.FAIL_ON_INVALID_CONFIG", False): | ||
| raise |
Contributor
There was a problem hiding this comment.
1. apply_mcp_server_config broad except 📘 Rule violation ☼ Reliability
apply_mcp_server_config() and _get_logger() catch Exception, which can mask unexpected bugs and make failures harder to debug. The compliance rules require targeted exceptions and preserving error context rather than broad catches.
Agent Prompt
## Issue description
`pr_agent/config_loader.py` uses broad `except Exception` in `_get_logger()` and `apply_mcp_server_config()`, which can hide real bugs and complicate debugging.
## Issue Context
Compliance requires catching only expected exception types and avoiding broad exception handlers.
## Fix Focus Areas
- pr_agent/config_loader.py[67-77]
- pr_agent/config_loader.py[176-192]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+77
| class DummyLogger: | ||
| def debug(self, *args, **kwargs): pass | ||
| def info(self, *args, **kwargs): pass | ||
| def warning(self, *args, **kwargs): pass | ||
| def error(self, *args, **kwargs): pass | ||
| return DummyLogger() |
Contributor
There was a problem hiding this comment.
2. dummylogger violates ruff e701 📘 Rule violation ⚙ Maintainability
DummyLogger defines methods as single-line def ...: pass, which is flagged by Ruff (E701) and can fail CI linting. Repository lint/format compliance requires new code to pass Ruff checks.
Agent Prompt
## Issue description
The new `DummyLogger` methods are written as `def ...: pass` on one line, which Ruff flags (E701).
## Issue Context
Ruff linting is enforced via `pyproject.toml` and must pass in CI.
## Fix Focus Areas
- pr_agent/config_loader.py[72-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 4faffbe |
| else: | ||
| response, finish_reason = await self.ai_handler.chat_completion( | ||
| model=model, temperature=get_settings().config.temperature, system=system_prompt, user=user_prompt) | ||
| img_path = self.vars['img_path'] if 'img_path' in variables else None |
Contributor
There was a problem hiding this comment.
1. self.vars['img_path'] may keyerror 📘 Rule violation ☼ Reliability
The new img_path assignment checks membership in variables but indexes into self.vars, which can raise KeyError when those dicts are out of sync. This violates defensive input validation expectations and can crash /ask flows.
Agent Prompt
## Issue description
`img_path` is derived by indexing `self.vars['img_path']` after checking `'img_path' in variables`, which can still raise `KeyError`.
## Issue Context
The key presence check is performed on a different dict (`variables`) than the one being indexed (`self.vars`).
## Fix Focus Areas
- pr_agent/tools/pr_questions.py[110-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+249
to
+254
| content_length_value = headers.get("content-length") | ||
| if not content_length_value: | ||
| raise MCPRuntimeError(f"MCP server '{self.server_name}' response missing Content-Length") | ||
|
|
||
| content_length = int(content_length_value) | ||
| body = self.process.stdout.read(content_length) |
Contributor
There was a problem hiding this comment.
2. Invalid content-length can crash 📘 Rule violation ☼ Reliability
MCPStdioClient._read_message casts Content-Length to int without handling ValueError, which can raise an unhandled exception on malformed server output. This violates robust error handling for external inputs.
Agent Prompt
## Issue description
Parsing `Content-Length` can raise `ValueError` if the header is malformed, causing an unhandled exception.
## Issue Context
This code consumes external server output and should fail with a clear `MCPRuntimeError` using exception chaining.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[249-262]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+458
to
+517
| def _send_request(self, method: str, params: dict[str, Any]) -> dict[str, Any]: | ||
| self._request_id += 1 | ||
| request_id = self._request_id | ||
| payload = { | ||
| "jsonrpc": "2.0", | ||
| "id": request_id, | ||
| "method": method, | ||
| "params": params, | ||
| } | ||
| try: | ||
| response = self._session.post( | ||
| self.url, | ||
| json=payload, | ||
| headers=self._build_extra_headers(), | ||
| timeout=self.timeout, | ||
| stream=True, | ||
| ) | ||
| response.raise_for_status() | ||
| except requests.RequestException as exc: | ||
| raise MCPRuntimeError( | ||
| f"MCP streamable HTTP request failed for '{self.server_name}': {exc}" | ||
| ) from exc | ||
|
|
||
| # Capture session ID returned on the initialize response. | ||
| session_id = response.headers.get("Mcp-Session-Id") | ||
| if session_id and not self._session_id: | ||
| self._session_id = session_id | ||
|
|
||
| content_type = response.headers.get("Content-Type", "") | ||
| if "text/event-stream" in content_type: | ||
| return self._parse_sse_response(response, request_id, method) | ||
| return self._parse_json_response(response, method) | ||
|
|
||
| def _parse_json_response(self, response: "requests.Response", method: str) -> dict[str, Any]: | ||
| try: | ||
| body = response.json() | ||
| except ValueError as exc: | ||
| raise MCPRuntimeError( | ||
| f"MCP streamable HTTP response is not valid JSON for '{self.server_name}'" | ||
| ) from exc | ||
| return self._extract_result(body, method) | ||
|
|
||
| def _parse_sse_response( | ||
| self, response: "requests.Response", request_id: int, method: str | ||
| ) -> dict[str, Any]: | ||
| """Read an SSE stream and return the JSON-RPC result that matches *request_id*.""" | ||
| try: | ||
| for raw_line in response.iter_lines(decode_unicode=True): | ||
| if not raw_line or not raw_line.startswith("data:"): | ||
| continue | ||
| data = raw_line[5:].lstrip(" ") | ||
| if not data: | ||
| continue | ||
| try: | ||
| message = json.loads(data) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if isinstance(message, dict) and message.get("id") == request_id: | ||
| return self._extract_result(message, method) | ||
| except requests.RequestException as exc: |
Contributor
There was a problem hiding this comment.
3. Streamable http response leak 🐞 Bug ☼ Reliability
MCPStreamableHttpClient._send_request() uses stream=True but never closes the requests.Response, and _parse_sse_response() can return early on a matching event without releasing the connection. This can leak connections and exhaust the session pool, breaking subsequent MCP calls.
Agent Prompt
### Issue description
Streamable HTTP MCP requests are made with `stream=True` but the response object is never closed, which can leak connections.
### Issue Context
- `_send_request()` returns from parsing methods without ensuring the response is closed.
- `_parse_sse_response()` returns early on a match, which is especially likely to leak.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[458-489]
- pr_agent/mcp/runtime.py[500-517]
### Suggested fix approach
- Ensure `response.close()` is called in a `finally` block in `_send_request()` after parsing completes.
- For SSE, close the response before returning a matching result (or structure `_send_request()` as:
- create response
- try parse
- finally close response).
- Consider also explicitly consuming/closing SSE streams when returning early.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit ee26abf |
Comment on lines
+210
to
+215
| def reader(): | ||
| try: | ||
| response_holder["message"] = self._read_message() | ||
| except BaseException as exc: # noqa: BLE001 | ||
| error_holder["error"] = exc | ||
|
|
Contributor
There was a problem hiding this comment.
1. baseexception caught in stdio reader 📘 Rule violation ☼ Reliability
_read_message_with_timeout() catches BaseException, which can swallow critical control-flow exceptions (e.g., KeyboardInterrupt, SystemExit) and violates targeted exception handling guidance. This reduces reliability and observability of failures.
Agent Prompt
## Issue description
The stdio reader thread catches `BaseException`, which may swallow `SystemExit`/`KeyboardInterrupt` and violates the requirement to catch only expected exception types.
## Issue Context
Prefer catching `Exception` (or narrower) and converting expected failures into `MCPRuntimeError` with chaining where appropriate.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[210-215]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 5df9224 |
Comment on lines
+730
to
+737
| def _resolve_config_values(self, value: Any) -> Any: | ||
| if isinstance(value, str): | ||
| return os.path.expanduser(os.path.expandvars(value)) | ||
| if isinstance(value, list): | ||
| return [self._resolve_config_values(item) for item in value] | ||
| if isinstance(value, dict): | ||
| return {key: self._resolve_config_values(item) for key, item in value.items()} | ||
| return value |
Contributor
There was a problem hiding this comment.
1. resolve_env_vars setting ignored 📘 Rule violation ⚙ Maintainability
The MCP config setting mcp.resolve_env_vars is added but the runtime always expands environment variables in _resolve_config_values(), making this behavior effectively hardcoded. This violates the requirement that runtime behavior be configurable via .pr_agent.toml/pr_agent/settings/ rather than embedded in code.
Agent Prompt
## Issue description
`mcp.resolve_env_vars` is introduced in settings, but the MCP runtime always expands env vars and `~` paths in `_resolve_config_values()`. This makes the behavior non-configurable and inconsistent with the new setting.
## Issue Context
The setting exists in `pr_agent/settings/configuration.toml` as `resolve_env_vars = true`, implying users can disable it. The runtime should read `get_settings().get("MCP.RESOLVE_ENV_VARS", True)` (or equivalent) and only expand variables when enabled.
## Fix Focus Areas
- pr_agent/settings/configuration.toml[73-80]
- pr_agent/mcp/runtime.py[730-737]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+105
to
+117
| tool_name = str(parsed_response.get("tool", "")).strip() | ||
| arguments = parsed_response.get("arguments") or {} | ||
| if not tool_name: | ||
| self._logger.warning("MCP tool orchestration returned an empty tool name; aborting tool loop") | ||
| return response_text, finish_reason | ||
| if not isinstance(arguments, dict): | ||
| self._logger.warning("MCP tool orchestration arguments must be a JSON object; aborting tool loop") | ||
| return response_text, finish_reason | ||
|
|
||
| try: | ||
| tool_result = tool_executor(tool_name, arguments) | ||
| if inspect.isawaitable(tool_result): | ||
| tool_result = await tool_result |
Contributor
There was a problem hiding this comment.
2. Tool allowlist bypass 🐞 Bug ⛨ Security
BaseAiHandler.chat_completion_with_tools executes whatever tool name the model returns without validating it against the advertised tool catalog, allowing invocation of tools that were intentionally omitted due to max_tools/max_schema_chars budgets. This can bypass tool exposure controls and trigger unexpected tool calls on configured MCP servers.
Agent Prompt
### Issue description
`BaseAiHandler.chat_completion_with_tools()` executes the model-provided `tool` name without validating it against the tool catalog passed in via `tools`. With MCP enabled, `runtime.build_tool_schemas()` may omit tools due to `max_tools` / `max_schema_chars`, but `runtime.create_tool_executor()` can still call any tool on configured servers, so a model can bypass the catalog exposure/budget by naming an omitted tool.
### Issue Context
- The `tools` catalog is used only to generate the system prompt.
- The MCP executor ultimately calls `MCPRuntime.call_tool()` and does not check whether a tool was advertised.
### Fix approach
Implement an allowlist of tool names that were actually advertised and refuse (or return a controlled error result for) calls not in that allowlist.
Recommended implementation (defense-in-depth):
1. In `BaseAiHandler.chat_completion_with_tools`, derive `allowed_tool_names` from the `tools` argument (support both shapes used in repo/tests):
- OpenAI-style: `tool["function"]["name"]`
- Simple style: `tool["name"]`
2. Before calling `tool_executor`, verify `tool_name in allowed_tool_names`.
- If not allowed: log a warning and set `tool_result` to a deterministic error like `"Tool not available: ..."` (and continue the loop / decrement the turn budget), or abort the loop.
3. Optionally, also wrap `runtime.create_tool_executor()` in `maybe_chat_completion_with_mcp` with an allowlist check to ensure enforcement even if other callers bypass the handler-level check.
### Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[58-129]
- pr_agent/mcp/integration.py[35-63]
- pr_agent/mcp/runtime.py[656-714]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 7a7991a |
Comment on lines
+120
to
+126
| try: | ||
| tool_result = tool_executor(tool_name, arguments) | ||
| if inspect.isawaitable(tool_result): | ||
| tool_result = await tool_result | ||
| except Exception as exc: # noqa: BLE001 | ||
| self._logger.warning("MCP tool '%s' raised an exception: %s", tool_name, exc) | ||
| tool_result = f"Tool error: {exc}" |
Contributor
There was a problem hiding this comment.
1. Broad except exception in tool loop 📘 Rule violation ☼ Reliability
chat_completion_with_tools() catches Exception around tool_executor(...), which can mask unexpected bugs and makes failures harder to debug. The compliance checklist requires targeted exceptions and proper exception handling patterns.
Agent Prompt
## Issue description
`chat_completion_with_tools()` wraps tool execution with a broad `except Exception`, which can hide unexpected errors and violates the targeted-exception requirement.
## Issue Context
The tool executor is an external boundary and should only handle expected failure modes (e.g., runtime/tool errors, validation/type errors), while letting unexpected exceptions surface (or be wrapped with explicit chaining) for debuggability.
## Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[120-126]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+613
to
+626
| def connect_server(self, server_name: str): | ||
| if not self.enabled: | ||
| raise MCPRuntimeError("MCP runtime is disabled") | ||
| if server_name in self._clients: | ||
| return | ||
|
|
||
| server_config = self._servers_config.get(server_name) | ||
| if not isinstance(server_config, dict): | ||
| raise MCPRuntimeError(f"MCP server '{server_name}' config must be an object") | ||
|
|
||
| client = self._build_client(server_name, self._resolve_config_values(server_config)) | ||
| client.connect() | ||
| self._clients[server_name] = client | ||
| self._logger.info(f"Connected MCP server '{server_name}'") |
Contributor
There was a problem hiding this comment.
2. Session leak on connect fail 🐞 Bug ☼ Reliability
MCPRuntime.connect_server() calls client.connect() before registering the client in _clients; if connect() raises (e.g., unreachable HTTP server), the created client is dropped and its underlying requests.Session is never closed, leaking sockets/file descriptors over repeated requests.
Agent Prompt
### Issue description
`MCPRuntime.connect_server()` can leak resources when `client.connect()` raises because the client is not yet tracked in `_clients`, so its `close()` method is never called.
### Issue Context
This is particularly impactful for `MCPHttpClient` / `MCPStreamableHttpClient`, which allocate a `requests.Session()` in `__init__` and rely on `close()` to release sockets.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[613-626]
- pr_agent/mcp/runtime.py[288-319]
- pr_agent/mcp/runtime.py[398-436]
### Implementation direction
Wrap `client.connect()` in `try/except`, and call `client.close()` on failure before re-raising. Only add the client to `_clients` after a successful connect.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 288e518 |
Comment on lines
+1
to
+12
| import json | ||
| import os | ||
| import subprocess | ||
| import threading | ||
| from abc import ABC, abstractmethod | ||
| from dataclasses import dataclass | ||
| from typing import Any, Optional | ||
|
|
||
| import requests | ||
|
|
||
| from pr_agent.config_loader import get_settings | ||
|
|
Contributor
There was a problem hiding this comment.
1. Missing requests dependency 🐞 Bug ☼ Reliability
pr_agent/mcp/runtime.py imports and uses the third-party requests library, but requirements.txt does not declare it. In deployments that install only declared dependencies, importing MCP integration will raise ImportError, breaking /ask, /review, and /improve even if MCP is disabled at runtime.
Agent Prompt
### Issue description
`pr_agent/mcp/runtime.py` now imports and uses `requests`, but `requests` is not declared in the project dependencies. This can cause `ImportError: No module named 'requests'` in clean/strict installs and break PR-Agent commands that import MCP integration.
### Issue Context
- `pr_agent/mcp/integration.py` imports `MCPRuntime` from `pr_agent/mcp/runtime.py`.
- Core tools (e.g., reviewer/questions/code suggestions) import `maybe_chat_completion_with_mcp`, so the runtime import happens on module import, not only when MCP is enabled.
### Fix Focus Areas
- requirements.txt[1-45]
- pr_agent/mcp/runtime.py[1-12]
- pr_agent/mcp/integration.py[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit d00b274 |
Comment on lines
+83
to
+112
| def __init__(self, server_name: str, config: dict[str, Any]): | ||
| super().__init__(server_name, config) | ||
| self.process: Optional[subprocess.Popen] = None | ||
| self._request_id = 0 | ||
| self.timeout = float(config.get("timeout", 30)) | ||
|
|
||
| def connect(self): | ||
| command = self.config.get("command") | ||
| if not command: | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' is missing 'command'") | ||
|
|
||
| args = self.config.get("args") or [] | ||
| if not isinstance(args, list): | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' args must be a list") | ||
|
|
||
| env = os.environ.copy() | ||
| server_env = self.config.get("env") or {} | ||
| if not isinstance(server_env, dict): | ||
| raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' env must be an object") | ||
| env.update({str(k): str(v) for k, v in server_env.items()}) | ||
|
|
||
| cwd = self.config.get("cwd") | ||
| self.process = subprocess.Popen( | ||
| [command, *args], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, | ||
| env=env, | ||
| cwd=cwd, | ||
| ) |
Contributor
There was a problem hiding this comment.
1. Unhandled mcp config types 🐞 Bug ☼ Reliability
MCP client initialization can raise uncaught ValueError/TypeError for malformed config (e.g., non-numeric timeout or non-string args elements), which are not wrapped as MCPRuntimeError and are not caught in tool discovery/orchestration. This can crash MCP tool discovery (build_tool_schemas) and therefore break /ask, /review, and /improve when MCP is enabled.
Agent Prompt
### Issue description
Malformed MCP server config values can raise low-level exceptions (`ValueError`/`TypeError`) during client construction/connection (e.g., `float(timeout)` or invalid `args` element types). These are not consistently wrapped as `MCPRuntimeError` and can escape tool discovery/orchestration, breaking MCP-enabled commands.
### Issue Context
- `MCPStdioClient.__init__` uses `float(config.get("timeout", 30))`.
- `MCPStdioClient.connect` passes `[command, *args]` to `subprocess.Popen` after only validating `args` is a list.
- `list_all_tools` only catches `MCPRuntimeError` and `OSError`.
- `maybe_chat_completion_with_mcp` does not catch exceptions around `runtime.build_tool_schemas(...)`.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[82-112]
- pr_agent/mcp/runtime.py[288-301]
- pr_agent/mcp/runtime.py[645-655]
- pr_agent/mcp/integration.py[24-43]
### Implementation notes
- Wrap timeout parsing in `try/except (TypeError, ValueError)` and raise `MCPRuntimeError` with server context.
- Validate/coerce `args` list items to `str` (or `os.PathLike`) before calling `Popen`; raise `MCPRuntimeError` on invalid types.
- Ensure discovery paths only need to catch `MCPRuntimeError` (i.e., normalize client-side errors into MCPRuntimeError), or broaden the catch in `list_all_tools` to prevent crashes while logging the failure.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit f7fa86b |
Comment on lines
+1
to
+4
| import pytest | ||
|
|
||
| from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler | ||
|
|
Contributor
There was a problem hiding this comment.
1. Unused pytest import 📘 Rule violation ⚙ Maintainability
tests/unittest/test_ai_handler_tool_orchestration.py imports pytest but never uses it, which will be flagged by repository linting and may fail CI. This violates the requirement to keep modified files compliant with lint/format tooling.
Agent Prompt
## Issue description
The new unit test file imports `pytest` but does not use it, which will trigger unused-import lint errors.
## Issue Context
`pytest` is not referenced in this file (no `pytest.mark`, `pytest.raises`, etc.).
## Fix Focus Areas
- tests/unittest/test_ai_handler_tool_orchestration.py[1-4]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+64
to
+71
| tool_call_example = json.dumps( | ||
| { | ||
| "type": "tool_call", | ||
| "tool": "server.tool", | ||
| "arguments": {"example": "value"}, | ||
| }, | ||
| separators=(",", ":"), | ||
| ).replace("{\"example\":\"value\"}", "{...}") |
Contributor
There was a problem hiding this comment.
2. Tool prompt invalid json 🐞 Bug ≡ Correctness
BaseAiHandler.chat_completion_with_tools tells the model to output JSON “exactly in this shape”, but
the tool-call example is mutated to include {...}, which is not valid JSON and can lead to
unparsable tool calls so the tool loop silently falls back to plain text responses.
Agent Prompt
### Issue description
`chat_completion_with_tools()` instructs the model to output JSON exactly matching an example, but the example is not valid JSON because it includes `{...}`. This can cause the model to emit invalid JSON tool calls that `_parse_tool_or_final_response()` cannot parse, disabling tool orchestration.
### Issue Context
The prompt text says “ONLY a JSON object exactly in this shape”, so the example must itself be valid JSON.
### Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[63-88]
- pr_agent/algo/ai_handlers/base_ai_handler.py[179-196]
### What to change
- Remove the `.replace(..., "{...}")` so the example remains valid JSON (e.g., keep `{"arguments":{"example":"value"}}`).
- If you want to communicate “arbitrary arguments”, adjust the instruction text (in plain English) rather than embedding invalid JSON.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+43
| max_tools = _get_tool_budget("MCP.MAX_TOOL_CATALOG_TOOLS", 12) | ||
| max_schema_chars = _get_tool_budget("MCP.MAX_TOOL_CATALOG_SCHEMA_CHARS", 12000) | ||
|
|
||
| tools = runtime.build_tool_schemas( | ||
| max_tools=max_tools, | ||
| max_schema_chars=max_schema_chars, | ||
| include_server_prefix=True, | ||
| ) | ||
| if not tools: |
Contributor
There was a problem hiding this comment.
3. Blocking tool discovery i/o 🐞 Bug ➹ Performance
maybe_chat_completion_with_mcp calls runtime.build_tool_schemas() directly in an async path, but build_tool_schemas() performs synchronous tool discovery (connect + list_tools) using requests/subprocess, which can block the event loop and delay webhook/background task processing.
Agent Prompt
### Issue description
Tool discovery (connect + tools/list) is executed synchronously inside `maybe_chat_completion_with_mcp()`’s async flow. Because discovery uses `requests` and (for stdio) `subprocess`, it can block the event loop and stall handling of other requests/background tasks.
### Issue Context
Tool execution is already offloaded via `create_tool_executor()` using `run_in_executor`, but discovery is not.
### Fix Focus Areas
- pr_agent/mcp/integration.py[15-75]
- pr_agent/mcp/runtime.py[672-712]
- pr_agent/mcp/runtime.py[342-409]
### What to change
Choose one:
1) In `maybe_chat_completion_with_mcp`, wrap `runtime.build_tool_schemas(...)` in `await loop.run_in_executor(...)`.
2) Add an async discovery method on `MCPRuntime` (e.g., `async_build_tool_schemas`) that runs `list_all_tools()` / `build_tool_schemas()` in an executor.
Ensure both connect/list_tools and schema-building are included in the offloaded work so no requests/subprocess calls happen on the event loop thread.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 04c884d |
Comment on lines
+194
to
+200
| def _resolve_mcp_config_path() -> Path: | ||
| env_path = os.getenv(MCP_CONFIG_ENV_VAR) | ||
| if env_path: | ||
| return Path(env_path).expanduser() | ||
| configured_path = get_settings().get("MCP.CONFIG_PATH") | ||
| return Path(str(configured_path)).expanduser() | ||
|
|
Contributor
There was a problem hiding this comment.
1. _resolve_mcp_config_path() accepts none 📘 Rule violation ☼ Reliability
_resolve_mcp_config_path() converts an unset MCP.CONFIG_PATH setting to Path('None'), which
can cause confusing runtime behavior and prevent MCP config loading from working as intended.
Settings-derived values should be defaulted/validated before use.
Agent Prompt
## Issue description
`_resolve_mcp_config_path()` can return `Path('None')` when `MCP.CONFIG_PATH` is unset/None, instead of a sensible default path.
## Issue Context
This path is used by `apply_mcp_server_config()` to decide whether to load MCP servers; a `None`-derived path can silently disable expected behavior.
## Fix Focus Areas
- pr_agent/config_loader.py[194-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+775
to
+787
| def _split_tool_name(self, tool_name: str) -> tuple[str, str]: | ||
| if "." in tool_name: | ||
| server_name, tool_short_name = tool_name.split(".", 1) | ||
| if server_name and tool_short_name: | ||
| return server_name, tool_short_name | ||
|
|
||
| if len(self._servers_config) == 1: | ||
| server_name = next(iter(self._servers_config.keys())) | ||
| return server_name, tool_name | ||
|
|
||
| raise MCPRuntimeError( | ||
| f"Tool name '{tool_name}' must use the '<server>.<tool>' form when multiple MCP servers are configured" | ||
| ) |
Contributor
There was a problem hiding this comment.
2. Dot server name ambiguity 🐞 Bug ≡ Correctness
MCP tool names are constructed as "<server>.<tool>", but _split_tool_name splits on the first '.', so servers whose names contain '.' will be parsed incorrectly and tool calls can be routed to the wrong server/tool or fail when multiple servers are configured.
Agent Prompt
## Issue description
MCP tool naming uses `f"{server_name}.{tool_name}"` and later parses tool calls via `split(".", 1)`. If a configured server name contains a dot (e.g., `"my.server"`), tool calls like `"my.server.echo"` are misparsed as server=`"my"`, tool=`"server.echo"`, causing wrong routing or failures.
## Issue Context
This affects both schema advertisement (`MCPToolDefinition.to_openai_tool`) and execution (`MCPRuntime._split_tool_name`).
## Fix Focus Areas
- pr_agent/mcp/runtime.py[47-56]
- pr_agent/mcp/runtime.py[775-787]
## Implementation direction
- Introduce an unambiguous encoding or mapping from advertised tool name -> (server_name, tool_name).
- Example approaches: maintain a dict built during `build_tool_schemas()` mapping the exact advertised function name to server/tool; or escape/encode server names before concatenation and reverse it in `_split_tool_name`.
- Add a unit test covering a server name containing '.' to prevent regressions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Introduce server-side MCP config loading from /etc/pr-agent/mcp.json or MCP_CONFIG_PATH, including JSONC parsing and VS Code / Claude schema normalization. Add the MCP runtime, HTTP and stdio clients, structured tool-calling orchestration on the base AI handler, and wire /ask, /review, and /improve through the MCP-aware integration helper. Expose MCP runtime status in /config output, document the configuration flow and AWS Knowledge example, and add focused tests for config loading, runtime behavior, tool orchestration, integration, and discovery.
Contributor
|
Persistent review updated to latest commit 6f0b13d |
| @@ -0,0 +1,388 @@ | |||
| import io | |||
Contributor
There was a problem hiding this comment.
1. Unused io import 📘 Rule violation ⚙ Maintainability
io is imported but never used, which will be flagged by Ruff and can fail CI pre-commit checks. This adds unnecessary noise and can mask other lint issues.
Agent Prompt
## Issue description
`tests/unittest/test_mcp_runtime.py` imports `io` but never uses it, which violates lint/pre-commit hygiene.
## Issue Context
Ruff will flag this as an unused import and may fail CI.
## Fix Focus Areas
- tests/unittest/test_mcp_runtime.py[1-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+135
to
+152
| try: | ||
| init_result = self._send_request( | ||
| "initialize", | ||
| { | ||
| "protocolVersion": self.config.get("protocol_version", "2024-11-05"), | ||
| "capabilities": self.config.get("client_capabilities", {}), | ||
| "clientInfo": self.config.get( | ||
| "client_info", | ||
| {"name": "pr-agent", "version": "mcp-runtime"}, | ||
| ), | ||
| }, | ||
| ) | ||
| self.server_capabilities = init_result.get("capabilities", {}) | ||
| self._send_notification("notifications/initialized", {}) | ||
| except Exception: | ||
| self._terminate_process() | ||
| self.process = None | ||
| raise |
Contributor
There was a problem hiding this comment.
2. mcpstdioclient.connect catches exception 📘 Rule violation ☼ Reliability
MCPStdioClient.connect() uses a broad except Exception which can mask unexpected failures and violates the targeted-exception requirement. This reduces debuggability and can hide programming errors.
Agent Prompt
## Issue description
`MCPStdioClient.connect()` catches `Exception` broadly during initialization, which can swallow unexpected errors.
## Issue Context
This code path is a recoverable flow where specific exception types are expected (I/O, subprocess, MCP protocol/runtime errors). The current pattern reduces debuggability and violates the compliance rule.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[135-152]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+648
to
+653
| client = self._build_client(server_name, self._resolve_config_values(server_config)) | ||
| try: | ||
| client.connect() | ||
| except Exception: | ||
| client.close() | ||
| raise |
Contributor
There was a problem hiding this comment.
3. connect_server catches exception 📘 Rule violation ☼ Reliability
MCPRuntime.connect_server() catches a broad Exception when connecting a client, which can hide unexpected errors and violates the targeted-exception requirement. It should catch only expected connection-related exception types (and still perform cleanup).
Agent Prompt
## Issue description
`MCPRuntime.connect_server()` uses `except Exception` around `client.connect()`, which is too broad and can mask bugs.
## Issue Context
Cleanup (`client.close()`) is important, but the handler should only catch expected exception types (e.g., MCPRuntimeError, OSError / transport errors) and preserve context.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[648-653]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+233
to
+248
| config_path = _resolve_mcp_config_path() | ||
| fail_on_invalid = bool(get_settings().get("MCP.FAIL_ON_INVALID_CONFIG", False)) | ||
| try: | ||
| if not config_path.exists(): | ||
| logger.debug(f"MCP config file not found, skipping load: {config_path}") | ||
| return | ||
| config_data = load_mcp_server_config(config_path) | ||
| settings = get_settings() | ||
| settings.set("MCP.SERVERS", config_data["servers"], merge=False) | ||
| settings.set("MCP.SERVER_CONFIG", config_data, merge=False) | ||
| settings.set("MCP.ACTIVE_CONFIG_PATH", str(config_path), merge=False) | ||
| logger.info(f"Loaded MCP server configuration from {config_path}") | ||
| except (ValueError, OSError, FileNotFoundError) as exc: | ||
| logger.error(f"Failed to load MCP server configuration from {config_path}: {exc}") | ||
| if fail_on_invalid: | ||
| raise |
Contributor
There was a problem hiding this comment.
4. Bool flags misparsed 🐞 Bug ≡ Correctness
apply_mcp_server_config() and MCPRuntime convert settings with bool(...), so string values like "false" become True and can unintentionally enable MCP or force FAIL_ON_INVALID_CONFIG behavior. Since the repo explicitly disables Dynaconf auto-casting, environment-provided overrides can flip MCP behavior and cause unexpected load failures.
Agent Prompt
### Issue description
MCP flag settings are parsed via `bool(...)`, which treats any non-empty string (including `'false'`) as `True`. Because this repo disables Dynaconf auto-casting, environment overrides can be strings and will be misinterpreted.
### Issue Context
This can unexpectedly enable MCP, expand env vars, or force `FAIL_ON_INVALID_CONFIG` behavior.
### Fix Focus Areas
- pr_agent/config_loader.py[231-248]
- pr_agent/mcp/runtime.py[604-626]
### Suggested fix
- Introduce a small helper like `_coerce_bool(value, default)` that:
- returns the value if it is already a `bool`
- if `str`, maps common values (`"true"/"1"/"yes"/"on"` => True; `"false"/"0"/"no"/"off"/""` => False)
- otherwise falls back to `default` or `bool(value)` as appropriate
- Use it for `MCP.FAIL_ON_INVALID_CONFIG`, `MCP.RESOLVE_ENV_VARS`, and `MCP.ENABLED`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce server-side MCP config loading from /etc/pr-agent/mcp.json or MCP_CONFIG_PATH, including JSONC parsing and VS Code / Claude schema normalization. Add the MCP runtime, HTTP and stdio clients, structured tool-calling orchestration on the base AI handler, and wire /ask, /review, and /improve through the MCP-aware integration helper. Expose MCP runtime status in /config output, document the configuration flow and AWS Knowledge example, and add focused tests for config loading, runtime behavior, tool orchestration, integration, and discovery.