Implement MCP runtime with client support

pr_agent/mcp/runtime.py

Add MCP integration helper for commands

pr_agent/mcp/integration.py

Wire improve command through MCP integration

pr_agent/tools/pr_code_suggestions.py

Wire ask command through MCP integration

pr_agent/tools/pr_questions.py

Wire review command through MCP integration

pr_agent/tools/pr_reviewer.py

Add MCP runtime status to config output

pr_agent/tools/pr_config.py

Add MCP configuration settings section

pr_agent/settings/configuration.toml

Test tool orchestration loop and fallback

tests/unittest/test_ai_handler_tool_orchestration.py

Test MCP config loading and schema normalization

tests/unittest/test_config_loader_mcp.py

Test MCP integration with command handlers

tests/unittest/test_mcp_integration_helper.py

Test MCP runtime client management

tests/unittest/test_mcp_runtime.py

Test tool schema building and budgeting

tests/unittest/test_mcp_tool_discovery.py

Document MCP configuration and AWS example

docs/docs/usage-guide/additional_configurations.md

Add MCP setup instructions to usage guide

docs/docs/usage-guide/automations_and_usage.md

Description

The new imports in base_ai_handler.py are not ordered/grouped as Ruff/isort expect, which can fail
linting and create noisy diffs. This violates the repo’s required formatting conventions.

Code

pr_agent/algo/ai_handlers/base_ai_handler.py[R1-7]

+import inspect
+import json
+import logging
from abc import ABC, abstractmethod
+from typing import Any, Awaitable, Callable, Optional
+
+from pr_agent.mcp.runtime import MCPRuntimeError

Evidence

Ruff/isort conventions require consistent import grouping/ordering. The changed import block places
from abc import ... after other stdlib imports, which is not isort-style ordering.

AGENTS.md
pr_agent/algo/ai_handlers/base_ai_handler.py[1-7]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The import block in `BaseAiHandler` is not ordered/grouped per Ruff/isort, which can fail CI lint checks.
## Issue Context
New imports were added and should follow the repository’s standard import ordering.
## Fix Focus Areas
- pr_agent/algo/ai_handlers/base_ai_handler.py[1-7]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

MCPRuntime.enabled reads settings via get_settings().get("MCP", {}) while other MCP settings use
dotted-key access (e.g., MCP.RESOLVE_ENV_VARS). This mixed access pattern risks inconsistent
behavior depending on Dynaconf shapes and violates the consistent configuration access requirement.

Code

pr_agent/mcp/runtime.py[R628-646]

+        self._resolve_env_vars = _parse_bool(get_settings().get("MCP.RESOLVE_ENV_VARS", True), default=True)
+        if servers_config is None:
+            servers_config = get_settings().get("MCP.SERVERS", {}) or {}
+
+        if not isinstance(servers_config, dict):
+            self._logger.warning("MCP.SERVERS is not an object; ignoring MCP server configuration")
+            servers_config = {}
+
+        self._servers_config = servers_config
+        self._clients: dict[str, BaseMCPClient] = {}
+
+    @property
+    def configured_server_names(self) -> list[str]:
+        return list(self._servers_config.keys())
+
+    @property
+    def enabled(self) -> bool:
+        policy_config = get_settings().get("MCP", {}) or {}
+        return _parse_bool(policy_config.get("ENABLED", False), default=False)

Evidence

The compliance checklist requires using one consistent settings access pattern/key path. In the MCP
runtime, _resolve_env_vars is read via get("MCP.RESOLVE_ENV_VARS", ...) but enabled is derived
by fetching the whole MCP object and indexing, mixing patterns.

pr_agent/mcp/runtime.py[628-646]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
MCP settings are accessed using mixed patterns (`get("MCP.X")` vs `get("MCP")` then `.get("X")`), which can cause inconsistent behavior and violates the repo’s configuration access conventions.
## Issue Context
`MCPRuntime.enabled` currently reads `MCP` as a dict, while other MCP settings are read via dotted keys.
## Fix Focus Areas
- pr_agent/mcp/runtime.py[628-646]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

A new [mcp] configuration section was added to the defaults, but the repository’s .pr_agent.toml
mirror was not updated to reflect the new behavior/config knobs. This can cause configuration
divergence and violates the configuration mirror update requirement.

Code

pr_agent/settings/configuration.toml[R73-79]

+[mcp]
+enabled = false
+config_path = "/etc/pr-agent/mcp.json"
+fail_on_invalid_config = false
+resolve_env_vars = true
+max_tool_catalog_tools = 12
+max_tool_catalog_schema_chars = 12000

Evidence

The checklist requires updating mirrored configuration locations together when behavior/config
changes. The PR adds MCP defaults in pr_agent/settings/configuration.toml, but .pr_agent.toml
contains no [mcp] section to mirror/advertise these options.

AGENTS.md
pr_agent/settings/configuration.toml[73-79]
.pr_agent.toml[1-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New MCP settings were added to the defaults, but `.pr_agent.toml` was not updated, which can cause mirrored config divergence.
## Issue Context
The repo uses both `pr_agent/settings/*.toml` and `.pr_agent.toml` as mirrored configuration surfaces.
## Fix Focus Areas
- pr_agent/settings/configuration.toml[73-79]
- .pr_agent.toml[1-20]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

On a stdio MCP timeout, MCPStdioClient terminates the subprocess but does not clear its process
handle (or force a close), and MCPRuntime keeps the client cached, so subsequent tool calls can
reuse a dead process and repeatedly fail. This makes tool execution non-recoverable after a single
timeout within the same runtime instance.

Code

pr_agent/mcp/runtime.py[R244-263]

+    def _read_message_with_timeout(self) -> dict[str, Any]:
+        response_holder: dict[str, Any] = {}
+        error_holder: dict[str, Exception] = {}
+
+        def reader():
+            try:
+                response_holder["message"] = self._read_message()
+            except MCPRuntimeError as exc:  # noqa: BLE001
+                error_holder["error"] = exc
+            except (OSError, ValueError, json.JSONDecodeError) as exc:  # noqa: BLE001
+                error_holder["error"] = exc
+
+        reader_thread = threading.Thread(target=reader, daemon=True)
+        reader_thread.start()
+        reader_thread.join(timeout=self.timeout)
+
+        if reader_thread.is_alive():
+            self._terminate_process()
+            raise MCPRuntimeError(f"Stdio MCP server '{self.server_name}' timed out waiting for a response")
+

Evidence

The timeout path kills the subprocess but leaves MCPStdioClient.process populated, and
MCPRuntime.call_tool() reuses the cached client from _clients without re-connecting or dropping it,
so later calls will continue hitting the broken client state.

pr_agent/mcp/runtime.py[244-270]
pr_agent/mcp/runtime.py[272-281]
pr_agent/mcp/runtime.py[757-769]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When a stdio MCP server times out, the runtime terminates the subprocess but keeps the client cached and leaves `MCPStdioClient.process` non-`None`. Subsequent tool calls then reuse a dead process and repeatedly fail.
### Issue Context
A timeout is an expected failure mode (slow/unresponsive MCP servers). The runtime should recover by clearing/closing the client and allowing a reconnect on the next tool call.
### Fix Focus Areas
- pr_agent/mcp/runtime.py[244-270]
- pr_agent/mcp/runtime.py[272-281]
- pr_agent/mcp/runtime.py[757-769]
### Implementation notes
- In the stdio timeout branch, call `self.close()` (or set `self.process = None` after terminating and ensure pipes are closed).
- Optionally, in `MCPRuntime.call_tool()`/executor path, catch `MCPRuntimeError`/`OSError` from `client.call_tool(...)` and `disconnect_server(server_name)` so the next call reconnects cleanly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

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.

Code

tests/unittest/test_mcp_runtime.py[1]

+import io

Evidence

The compliance checklist requires new/updated code to remain lint-clean (no unused imports). The
file imports io but does not reference it anywhere in the added test module.

tests/unittest/test_mcp_runtime.py[1-1]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R135-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

Evidence

The checklist requires targeted exception handling and discourages broad except Exception. The new
MCP stdio connection code catches Exception broadly during initialization.

pr_agent/mcp/runtime.py[135-152]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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).

Code

pr_agent/mcp/runtime.py[R648-653]

+        client = self._build_client(server_name, self._resolve_config_values(server_config))
+        try:
+            client.connect()
+        except Exception:
+            client.close()
+            raise

Evidence

The compliance checklist requires targeted exception handling. The new MCP runtime connection code
uses except Exception for cleanup, which is overly broad for a recoverable connect flow.

pr_agent/mcp/runtime.py[648-653]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/config_loader.py[R233-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

Evidence

The project sets AUTO_CAST_FOR_DYNACONF=false, which means Dynaconf may surface env overrides as
strings; bool("false") evaluates to True. The PR adds MCP logic that uses
bool(get_settings().get(...)) for MCP flags, so an env override like
MCP_FAIL_ON_INVALID_CONFIG="false" can be treated as True, and MCP_ENABLED="false" can be treated as
enabled.

pr_agent/log/init.py[1-2]
pr_agent/git_providers/utils.py[14-17]
pr_agent/config_loader.py[231-248]
pr_agent/mcp/runtime.py[604-626]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

_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.

Code

pr_agent/config_loader.py[R194-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()
+

Evidence

PR Compliance ID 15 requires validating/normalizing settings values before use. The added
_resolve_mcp_config_path() reads MCP.CONFIG_PATH without a default and then does
Path(str(configured_path)), so a missing setting becomes the literal path None rather than a
valid config path.

pr_agent/config_loader.py[194-200]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R775-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"
+        )

Evidence

Tool names embed the raw server name using a dot separator, while parsing assumes the first dot
separates server/tool. This makes names ambiguous when the server name itself contains dots.

pr_agent/mcp/runtime.py[47-56]
pr_agent/mcp/runtime.py[775-787]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

tests/unittest/test_ai_handler_tool_orchestration.py[R1-4]

+import pytest
+
+from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler
+

Evidence

PR Compliance ID 22 requires avoiding lint violations such as unused imports; the added test file
imports pytest but does not reference it anywhere in the file.

tests/unittest/test_ai_handler_tool_orchestration.py[1-4]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/algo/ai_handlers/base_ai_handler.py[R64-71]

+        tool_call_example = json.dumps(
+            {
+                "type": "tool_call",
+                "tool": "server.tool",
+                "arguments": {"example": "value"},
+            },
+            separators=(",", ":"),
+        ).replace("{\"example\":\"value\"}", "{...}")

Evidence

The system prompt example is made invalid by replacing the arguments object with {...}. The parser
only accepts json.loads()-parseable dicts with type tool_call/final, so if the model follows the
example literally the orchestration won’t recognize the tool request and will return raw model
output.

/pr_agent/algo/ai_handlers/base_ai_handler.py[63-88]
/pr_agent/algo/ai_handlers/base_ai_handler.py[179-196]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/integration.py[R35-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:

Evidence

In the async integration helper, tool schemas are built synchronously.
MCPRuntime.build_tool_schemas() calls list_all_tools(), which calls list_server_tools(), which
connects and calls client.list_tools(). For HTTP servers, list_tools ultimately calls
requests.Session.post synchronously. None of this is offloaded to a thread pool during discovery, so
it can block the asyncio loop.

/pr_agent/mcp/integration.py[15-73]
/pr_agent/mcp/runtime.py[672-712]
/pr_agent/mcp/runtime.py[342-409]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R83-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,
+        )

Evidence

MCPStdioClient casts timeout with float(...) and passes args directly into
subprocess.Popen([command, *args]) after only checking that args is a list (not that its members
are valid). These exceptions are not converted to MCPRuntimeError. During discovery,
list_all_tools only catches (MCPRuntimeError, OSError), and maybe_chat_completion_with_mcp
doesn’t catch exceptions around build_tool_schemas, so type errors can bubble up and fail the
command.

pr_agent/mcp/runtime.py[82-112]
pr_agent/mcp/runtime.py[645-655]
pr_agent/mcp/integration.py[24-43]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R1-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
+

Evidence

The MCP runtime unconditionally imports requests, MCP integration imports the runtime, and core
tools import the MCP integration helper; meanwhile, the repo dependency list does not include
requests, so a clean install can fail at import time.

pr_agent/mcp/runtime.py[1-12]
pr_agent/mcp/integration.py[1-6]
pr_agent/tools/pr_reviewer.py[19-27]
requirements.txt[1-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/algo/ai_handlers/base_ai_handler.py[R120-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}"

Evidence

PR Compliance ID 15 forbids broad except Exception; the added tool execution wrapper uses `except
Exception as exc (suppressed with # noqa: BLE001`).

pr_agent/algo/ai_handlers/base_ai_handler.py[120-126]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R613-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}'")

Evidence

The runtime builds a client and invokes connect() before storing it; HTTP-based clients allocate a
requests.Session() in __init__ and only release resources in close(). On a connect failure,
the runtime never tracks the client, so disconnect_all() cannot close it, and the session remains
open.

pr_agent/mcp/runtime.py[613-626]
pr_agent/mcp/runtime.py[288-319]
pr_agent/mcp/runtime.py[398-436]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/mcp/runtime.py[R730-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

Evidence

PR Compliance ID 7 requires configurable behavior to be controlled by settings, not hardcoded. The
PR adds mcp.resolve_env_vars in configuration.toml, but the code always performs
expandvars/expanduser without checking that setting.

AGENTS.md
pr_agent/settings/configuration.toml[73-80]
pr_agent/mcp/runtime.py[730-737]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

Description

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.

Code

pr_agent/algo/ai_handlers/base_ai_handler.py[R105-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)
+...