fix: MCP client lifecycle - thin core wiring, OAuth honesty, remote support#1777
fix: MCP client lifecycle - thin core wiring, OAuth honesty, remote support#1777praisonai-triage-agent[bot] wants to merge 1 commit into
Conversation
…upport Implements comprehensive fixes for MCP client integration addressing broken imports, misleading OAuth documentation, and missing remote server support. Adds thin wiring between config and agents following protocol-driven architecture. Tier A - Critical Fixes: - Fix MCPConfig.to_mcp_instance() import from praisonaiagents.mcp - Mark OAuth CLI commands as experimental until real implementation - Update doctor checks to use canonical ~/.praisonai/mcp/ paths Tier B - Core Agent Value: - Add MCPClientProtocol interface for extensibility - Create load_mcp_tools() helper for agent-centric tool loading - Wire tool filtering with allowed_tools/disabled_tools parameters - Add safe environment policy (baseline + explicit config only) - Prepare tool name collision handling framework Tier C - Wrapper Improvements: - Add remote server support to mcp test/sync CLI commands - Create YAML MCP configuration example Tests: All smoke tests pass, no performance regressions (13ms import) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR implements a comprehensive MCP client lifecycle overhaul: standardizes tool filtering via new ChangesMCP Client Lifecycle & Tool Filtering
Sequence Diagram(s)The PR modifies multiple distinct control flows (tool filtering, config discovery, CLI remote/local branching, environment handling). The most significant observable sequence is the tool filtering pipeline applied across transport initialization: sequenceDiagram
participant Agent as Agent Code
participant MCPInit as MCP.__init__
participant Transport as Transport Layer
participant FilterHelper as _apply_tool_filters
participant Env as _build_safe_env
Agent->>MCPInit: new MCP(..., allowed_tools=['tool1'], disabled_tools=['tool2'])
MCPInit->>Env: build_safe_env(custom_env)
Env-->>MCPInit: {PATH, HOME, custom vars}
MCPInit->>Transport: connect with safe env
Transport-->>MCPInit: [raw_tool1, raw_tool2, ...]
MCPInit->>FilterHelper: _apply_tool_filters(tools)
FilterHelper-->>MCPInit: [tool1] (filtered)
MCPInit->>Agent: tools ready (filtered)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: The PR spans heterogeneous changes across 5 main areas (MCP core filtering/env, public API/loader, CLI remote/local branching, config paths, tests), each requiring distinct reasoning. Core logic (filtering across transports, safe env construction) is moderate-density with new helpers. CLI changes involve conditional branching and HTTP interaction. All areas are tightly coupled (tests validate filtering, CLI calls loader, loader uses MCP with filters), so review cannot be partitioned fully. Import fixes and path canonicalization add minor overhead. Comprehensive test coverage mitigates complexity somewhat. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR addresses MCP client lifecycle issues: fixing a broken import in
Confidence Score: 3/5Two real behavioural defects in changed core paths need to be addressed before merging. The env-policy change in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[load_mcp_tools] --> B{configs injected?}
B -- yes --> C[filter by names if provided]
B -- no --> D[MCPConfigManager]
D --> E[get_enabled_configs / get_config]
C --> F[iterate configs]
E --> F
F --> G{config.enabled?}
G -- no --> H[skip]
G -- yes --> I[MCPConfig.to_mcp_instance]
I --> J[from praisonaiagents.mcp import MCP]
J --> K[MCP.__init__]
K --> L[_build_safe_env\nwhitelist + custom_env]
K --> M{transport type?}
M -- stdio/npx --> N[_generate_tool_functions]
M -- SSE --> O[SSEMCPClient]
M -- HTTP stream --> P[HTTPStreamMCPClient]
M -- WebSocket --> Q[WebSocketMCPClient]
N --> R[_apply_tool_filters]
O --> R
P --> R
Q --> R
R --> S{allowed_tools set?}
S -- yes --> T[filter_tools_by_allowlist]
S -- no --> U{disabled_tools set?}
T --> U
U -- yes --> V[filter_disabled_tools]
U -- no --> W[return tools]
V --> W
W --> X[Agent tools list]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with httpx.Client(timeout=timeout) as client: | ||
| response = client.get(server.url) | ||
|
|
||
| if response.status_code == 200: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "ok", "message": "Remote server reachable"}) | ||
| else: | ||
| output.print_success(f"Remote server '{name}' is reachable") | ||
| else: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "error", "message": f"HTTP {response.status_code}"}) | ||
| else: | ||
| output.print_error(f"Remote server returned HTTP {response.status_code}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
MCP remote servers communicate over SSE (/sse) or HTTP streaming (/mcp), not a bare GET on the root URL. A correctly-functioning server will often return 404 or 405 for a GET to /, causing this check to report failure even though the server is operational. Conversely, any HTTP server returning 200 (a proxy, a static page, a different service on the same host) will pass the check regardless of whether MCP is actually available. Consider probing a known MCP endpoint, or downgrade the probe to a pure TCP connect check that avoids false positives and negatives.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai-agents/praisonaiagents/mcp/mcp.py (1)
266-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same filters in
to_openai_tool().These assignments only trim
self._tools.to_openai_tool()still reads fromself.runner.toolsor the transport clients directly, so filtered-out tools reappear for agents that consume MCP through the OpenAI tool-definition path.Also applies to: 371-371, 491-491
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py` around lines 266 - 307, The MCP client builds filtered tool lists into self._tools via _apply_tool_filters, but to_openai_tool() still reads unfiltered sources (self.runner.tools or direct transport_client.tools), causing removed tools to reappear; update to_openai_tool() to read from self._tools (or call _apply_tool_filters on the transport/tool source before converting) so it always uses the filtered list, and apply the same fix for the websocket, SSE and HTTP-stream client code paths that populate tools (references: to_openai_tool, _apply_tool_filters, self._tools, self.runner.tools, self.websocket_client.tools, self.sse_client.tools, self.http_stream_client.tools); ensure any creation of OpenAI tool definitions uses the filtered list and cover the other occurrences called out (around the other two spots).src/praisonai/praisonai/cli/commands/mcp.py (1)
201-255:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: orphaned
exceptclauses — module will fail to import (SyntaxError).The local-branch
try:at Line 210 is indented underelse:(8 spaces) and has no matchingexceptat that level. The trailingexcept FileNotFoundError/except Exceptionat Lines 244 and 250 sit at 4-space indentation with notryat that level, so Python cannot parse this function. The remote branch (Lines 171–200) already carries its own self-containedtry/except, so these handlers belong inside the local branch'stry.🐛 Re-indent the trailing handlers into the local-branch try
- except FileNotFoundError: - if output.is_json_mode: - output.print_json({"name": name, "status": "error", "message": f"Command not found: {server.command}"}) - else: - output.print_error(f"Command not found: {server.command}") - raise typer.Exit(1) - except Exception as e: - if output.is_json_mode: - output.print_json({"name": name, "status": "error", "message": str(e)}) - else: - output.print_error(f"Test failed: {e}") - raise typer.Exit(1) + except FileNotFoundError: + if output.is_json_mode: + output.print_json({"name": name, "status": "error", "message": f"Command not found: {server.command}"}) + else: + output.print_error(f"Command not found: {server.command}") + raise typer.Exit(1) + except Exception as e: + if output.is_json_mode: + output.print_json({"name": name, "status": "error", "message": str(e)}) + else: + output.print_error(f"Test failed: {e}") + raise typer.Exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/cli/commands/mcp.py` around lines 201 - 255, The trailing except blocks are incorrectly indented and are orphaned from the try inside the local-server else branch, causing a SyntaxError; relocate/indent the except FileNotFoundError and except Exception handlers so they are nested under the same try that starts before launching the subprocess (the try that wraps cmd/env, subprocess.Popen, time.sleep, proc.poll(), proc.terminate(), etc.), keeping the existing logic that uses output.is_json_mode, output.print_json/print_error, and typer.Exit; ensure stderr reading (proc.stderr.read()) and proc.wait(timeout=2) remain within that try/except scope and that the except handlers reference the same local symbols (server.command, name, output, proc where relevant).
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/mcp/loader.py (1)
80-88: 💤 Low value
prefix_toolsis a no-op but the API/docstring promise collision avoidance.
prefix_toolsdefaults toTrueand the docstring states tool names are prefixed "to avoid collisions," yet this block only computesserver_nameand falls throughpass. With multiple servers exposing identically named tools, callers will silently get colliding tool names while believing collisions are handled.Until prefixing is wired, consider documenting the limitation in the docstring (or downgrading the promise) so callers don't rely on protection that isn't there.
Want me to open a tracking issue for wiring the prefix into the returned
MCPinstances?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/mcp/loader.py` around lines 80 - 88, The prefix_tools flag is currently a no-op: the block computing server_name (using config.name) does nothing and callers can get colliding tool names; update the loader function's docstring (the function that defines prefix_tools/target_configs and builds mcp_clients) to clearly state that prefix_tools is not yet applied and collisions are possible (or change the default to False), and either remove the misleading code block or replace the pass with a clear TODO comment referencing this limitation so callers are not misled; ensure you reference the variables prefix_tools, target_configs, config.name and the mcp object in the docstring/comment so future wiring of prefixing into the returned MCP instances is straightforward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/yaml/agent-with-mcp.yaml`:
- Line 36: Remove the unsupported description field from the MCPLocalConfig
instance in the YAML (the key named "description"); either delete the
"description: \"Time and date utilities\"" entry or turn it into a YAML comment,
ensuring the YAML conforms to the MCPLocalConfig schema and only uses supported
keys for the agent configuration.
- Line 27: Remove the unsupported "description" field from the MCP config entry
(it is not part of the MCPLocalConfig schema); either delete the description key
or convert it into a YAML comment, and ensure only supported keys remain: type,
command, args, env, enabled, timeout as defined by MCPLocalConfig.
- Around line 38-47: The commented example showing
mcp.servers.<name>.tools.include and tools.exclude is misleading because the MCP
server config types (MCPLocalConfig / MCPRemoteConfig) and
ConfigSchema.from_dict() do not support a tools/include/exclude field and sync
uses mcp.get_tools() to fetch all tools; either remove this commented block
entirely or replace it with a clear note stating that per-server tool
include/exclude is unsupported for MCP servers and instruct users to filter
tools elsewhere (e.g., after mcp.get_tools()), referencing the mcp.servers,
tools.include/tools.exclude symbols and the
MCPLocalConfig/MCPRemoteConfig/ConfigSchema.from_dict()/mcp.get_tools() APIs so
maintainers can find and update the snippet.
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py`:
- Around line 571-579: The Windows-specific safe_baseline update in mcp.py omits
SYSTEMROOT, which can break Python/child executables; modify the if block that
updates safe_baseline (the dict inside the platform.system() == 'Windows'
branch) to include 'SYSTEMROOT': os.environ.get('SYSTEMROOT', '') so the
environment baseline retains SYSTEMROOT for stdio MCP servers.
- Around line 529-535: The current filtering order removes tools present in both
allowed_tools and disabled_tools contrary to the documented precedence; change
the order so that filter_disabled_tools is applied first and then
filter_tools_by_allowlist is applied (i.e., call
filter_disabled_tools(tool_defs, self.disabled_tools) before calling
filter_tools_by_allowlist(tool_defs, self.allowed_tools)) so that allowed_tools
wins over disabled_tools; update the logic around the symbols tool_defs,
allowed_tools, disabled_tools, filter_disabled_tools and
filter_tools_by_allowlist in the mcp.MCP class accordingly.
In `@src/praisonai-agents/praisonaiagents/mcp/protocols.py`:
- Around line 22-58: The protocol currently only exposes blocking methods
(list_tools, get_tools, call_tool, shutdown); add async counterparts on the same
protocol class so implementations can provide non-blocking I/O: define
async_list_tools (async def returning List[Dict[str, Any]]), async_get_tools
(async alias to async_list_tools), async_call_tool (async def taking name: str,
args: Dict[str, Any] and returning Any, raising on failure), and async_shutdown
(async def returning None); ensure signatures and docstrings mirror their sync
equivalents and use asyncio-friendly return types so adapters can implement both
sync and async variants consistently.
In `@src/praisonai-agents/tests/unit/test_mcp_loader.py`:
- Around line 1-125: The test file lacks a mandatory agentic end-to-end test:
add a new test function (e.g., test_agent_with_mcp_tools_end_to_end) that
imports load_mcp_tools and Agent, obtains tools via load_mcp_tools(...),
constructs an Agent instance (Agent(..., tools=tools)), calls agent.start("a
prompt that exercises MCP tools"), and asserts the returned response is a
non-empty string (isinstance(response, str) and len(response) > 0); ensure the
test handles ImportError/skips if MCP package or LLM is unavailable but
otherwise runs a real agentic call using the Agent.start method.
In `@src/praisonai/praisonai/cli/commands/mcp.py`:
- Around line 212-221: The code currently replaces the child process environment
when server.env is set (env = dict(**server.env)), causing inherited vars like
PATH to be lost; modify the logic before subprocess.Popen to create a merged
environment by copying the current process environment (os.environ) and updating
it with server.env so that server.env augments/overrides defaults rather than
replaces them, then pass that merged env into the subprocess.Popen call that
uses cmd, stdin/stdout/stderr and env.
- Around line 178-200: The generic except block is catching the explicit raise
typer.Exit(1) from the HTTP non-200 path; update the exception handlers in the
function that performs the remote check so typer.Exit is allowed to propagate:
add a specific "except typer.Exit: raise" clause before the broad "except
Exception as e" (or, alternatively, inside the generic handler re-raise when
isinstance(e, typer.Exit)); keep the existing json/non-json logging in the other
handlers and only handle non-typer exceptions in the final except block.
In `@src/praisonai/praisonai/cli/features/doctor/checks/mcp_checks.py`:
- Around line 40-45: The fallback locations list in the branch used when
praisonaiagents is not available (the variable named locations) omits the
canonical project-level MCP path; add Path.cwd() / ".praisonai" / "mcp" /
"config.json" to that locations list so it matches the main branch's
project_mcp_dir discovery logic (keep the rest of the legacy `.praison`
fallbacks intact).
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py`:
- Around line 266-307: The MCP client builds filtered tool lists into
self._tools via _apply_tool_filters, but to_openai_tool() still reads unfiltered
sources (self.runner.tools or direct transport_client.tools), causing removed
tools to reappear; update to_openai_tool() to read from self._tools (or call
_apply_tool_filters on the transport/tool source before converting) so it always
uses the filtered list, and apply the same fix for the websocket, SSE and
HTTP-stream client code paths that populate tools (references: to_openai_tool,
_apply_tool_filters, self._tools, self.runner.tools,
self.websocket_client.tools, self.sse_client.tools,
self.http_stream_client.tools); ensure any creation of OpenAI tool definitions
uses the filtered list and cover the other occurrences called out (around the
other two spots).
In `@src/praisonai/praisonai/cli/commands/mcp.py`:
- Around line 201-255: The trailing except blocks are incorrectly indented and
are orphaned from the try inside the local-server else branch, causing a
SyntaxError; relocate/indent the except FileNotFoundError and except Exception
handlers so they are nested under the same try that starts before launching the
subprocess (the try that wraps cmd/env, subprocess.Popen, time.sleep,
proc.poll(), proc.terminate(), etc.), keeping the existing logic that uses
output.is_json_mode, output.print_json/print_error, and typer.Exit; ensure
stderr reading (proc.stderr.read()) and proc.wait(timeout=2) remain within that
try/except scope and that the except handlers reference the same local symbols
(server.command, name, output, proc where relevant).
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/mcp/loader.py`:
- Around line 80-88: The prefix_tools flag is currently a no-op: the block
computing server_name (using config.name) does nothing and callers can get
colliding tool names; update the loader function's docstring (the function that
defines prefix_tools/target_configs and builds mcp_clients) to clearly state
that prefix_tools is not yet applied and collisions are possible (or change the
default to False), and either remove the misleading code block or replace the
pass with a clear TODO comment referencing this limitation so callers are not
misled; ensure you reference the variables prefix_tools, target_configs,
config.name and the mcp object in the docstring/comment so future wiring of
prefixing into the returned MCP instances is straightforward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10617270-1108-4b05-840b-10b981e0f040
📒 Files selected for processing (9)
examples/yaml/agent-with-mcp.yamlsrc/praisonai-agents/praisonaiagents/mcp/__init__.pysrc/praisonai-agents/praisonaiagents/mcp/loader.pysrc/praisonai-agents/praisonaiagents/mcp/mcp.pysrc/praisonai-agents/praisonaiagents/mcp/protocols.pysrc/praisonai-agents/praisonaiagents/memory/mcp_config.pysrc/praisonai-agents/tests/unit/test_mcp_loader.pysrc/praisonai/praisonai/cli/commands/mcp.pysrc/praisonai/praisonai/cli/features/doctor/checks/mcp_checks.py
| - "." | ||
| env: {} | ||
| enabled: true | ||
| description: "Filesystem access for the agent" |
There was a problem hiding this comment.
Remove unsupported description field.
The description field is not part of the MCPLocalConfig schema (see src/praisonai/praisonai/cli/configuration/schema.py:34-44). The schema only supports: type, command, args, env, enabled, and timeout.
🔧 Proposed fix
enabled: true
- description: "Filesystem access for the agent"If you want to document the server purpose, use a YAML comment instead:
filesystem:
+ # Filesystem access for the agent
command: npx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: "Filesystem access for the agent" | |
| enabled: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/yaml/agent-with-mcp.yaml` at line 27, Remove the unsupported
"description" field from the MCP config entry (it is not part of the
MCPLocalConfig schema); either delete the description key or convert it into a
YAML comment, and ensure only supported keys remain: type, command, args, env,
enabled, timeout as defined by MCPLocalConfig.
| - "@modelcontextprotocol/server-time" | ||
| env: {} | ||
| enabled: true | ||
| description: "Time and date utilities" |
There was a problem hiding this comment.
Remove unsupported description field.
Same issue as line 27 - the description field is not part of the MCPLocalConfig schema.
🔧 Proposed fix
enabled: true
- description: "Time and date utilities"Or use a YAML comment:
time:
+ # Time and date utilities
command: npx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: "Time and date utilities" | |
| enabled: true |
| description: "Time and date utilities" | |
| time: | |
| # Time and date utilities | |
| enabled: true | |
| command: npx |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/yaml/agent-with-mcp.yaml` at line 36, Remove the unsupported
description field from the MCPLocalConfig instance in the YAML (the key named
"description"); either delete the "description: \"Time and date utilities\""
entry or turn it into a YAML comment, ensuring the YAML conforms to the
MCPLocalConfig schema and only uses supported keys for the agent configuration.
| # Optional: Tool filtering per server | ||
| # mcp: | ||
| # servers: | ||
| # filesystem: | ||
| # command: npx | ||
| # args: ["-y", "@modelcontextprotocol/server-filesystem", "."] | ||
| # tools: | ||
| # include: ["read_file", "write_file", "list_directory"] | ||
| # # exclude: ["delete_file"] | ||
| # enabled: true No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for schema definitions that might support tool filtering in MCP config
rg -n -C5 'class.*MCPLocalConfig|class.*MCPRemoteConfig' src/praisonai/praisonai/cli/configuration/schema.py
# Search for any config loading that handles 'tools', 'include', or 'exclude' fields
rg -n -C3 '\btools\b.*\binclude\b|\btools\b.*\bexclude\b|allowed_tools|disabled_tools' src/praisonai/praisonai/cli/configuration/Repository: MervinPraison/PraisonAI
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Inspect the example YAML around the referenced lines
sed -n '1,120p' examples/yaml/agent-with-mcp.yaml | nl -ba | sed -n '25,90p'
# Confirm MCP schema contents (already partially known) with exact lines for local/remote dataclasses
rg -n "class MCPLocalConfig|class MCPRemoteConfig" -n src/praisonai/praisonai/cli/configuration/schema.py
sed -n '1,120p' src/praisonai/praisonai/cli/configuration/schema.py | nl -ba | sed -n '20,90p'
# Find any config/YAML keys that might map to tool filtering anywhere in the CLI/config code
rg -n --hidden --no-ignore-vcs "allowed_tools|disabled_tools|tools\.include|tools\.exclude|\binclude\b.*tool|\bexclude\b.*tool|server-filesystem.*tools" src/praisonai/praisonai
# Find where MCP server configs are parsed/instantiated to see if any “tools” keys exist outside the schema file
rg -n --hidden --no-ignore-vcs "MCPLocalConfig|MCPRemoteConfig|mcp:\s*|servers:" src/praisonai/praisonai/cli/configuration src/praisonai/praisonaiRepository: MervinPraison/PraisonAI
Length of output: 12708
🏁 Script executed:
#!/bin/bash
set -e
# 1) Show the referenced YAML snippet (without nl)
sed -n '30,60p' examples/yaml/agent-with-mcp.yaml
# 2) Show schema.py around MCPLocalConfig/MCPRemoteConfig and how server configs are constructed
sed -n '1,240p' src/praisonai/praisonai/cli/configuration/schema.py
# 3) Look for any mcp server config handling of tools/include/exclude in parsing/commands
rg -n --hidden --no-ignore-vcs "mcp.*tools|mcp.*include|mcp.*exclude|tools\.include|tools\.exclude|allowed_tools|disabled_tools" src/praisonai/praisonai/cli/commands/mcp.py src/praisonai/praisonai/cli/features/mcp.py src/praisonai/praisonai/acp/server.py src/praisonai/praisonai/acp/session.py src/praisonai/praisonai/endpoints/__init__.py
# 4) Broader: find any occurrences of "tools:" under config schema/builders (limit to cli/configuration subtree)
rg -n --hidden --no-ignore-vcs "tools:" src/praisonai/praisonai/cli/configurationRepository: MervinPraison/PraisonAI
Length of output: 9405
🏁 Script executed:
#!/bin/bash
set -e
# Inspect relevant sections of MCP commands/features
sed -n '250,620p' src/praisonai/praisonai/cli/commands/mcp.py | nl -ba | sed -n '250,620p'
sed -n '1,320p' src/praisonai/praisonai/cli/features/mcp.py | nl -ba | sed -n '1,320p'
# Search for tool filtering / include / exclude handling in MCP code
rg -n "server_data\.get\\(\"tools\"\\)|\\btools\\b.*(include|exclude)|include.*exclude|allowed_tools|disabled_tools" src/praisonai/praisonai/cli/commands/mcp.py src/praisonai/praisonai/cli/features/mcp.py src/praisonai/praisonai/mcp_server -S
# Check whether config parsing validates keys (unknown keys) anywhere in configuration/ loading
rg -n "validate|validation|schema.*validate|additionalProperties|extra" src/praisonai/praisonai/cli/configuration -SRepository: MervinPraison/PraisonAI
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -e
# Use cat -n to show line numbers (nl not available)
sed -n '1,220p' src/praisonai/praisonai/cli/features/mcp.py | cat -n
echo "----"
sed -n '1,260p' src/praisonai/praisonai/cli/commands/mcp.py | cat -n
echo "----"
sed -n '260,620p' src/praisonai/praisonai/cli/commands/mcp.py | cat -n
# Search for include/exclude or per-MCP-server tool filtering
rg -n --hidden --no-ignore-vcs "(tools.*(include|exclude)|include.*exclude|allowed_tools|disabled_tools|server_data\.get\(\"tools\"\)|server.*tools)" src/praisonai/praisonai/cli/commands/mcp.py src/praisonai/praisonai/cli/features/mcp.py src/praisonai/praisonai/mcp_server -S
# Search where MCP config is parsed/validated in CLI config loading
rg -n --hidden --no-ignore-vcs "(from_dict\(|ConfigSchema|additionalProperties|validate|validation)" src/praisonai/praisonai/cli/configuration -SRepository: MervinPraison/PraisonAI
Length of output: 38359
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,240p' src/praisonai/praisonai/cli/configuration/loader.py | cat -n
echo "----"
sed -n '240,420p' src/praisonai/praisonai/cli/configuration/loader.py | cat -n
echo "----"
sed -n '420,520p' src/praisonai/praisonai/cli/configuration/loader.py | cat -n
# Search within loader for any strict validation / key rejection logic
rg -n --hidden --no-ignore-vcs "(validate|validation|jsonschema|additionalProperties|strict|unknown|key.*error|raise .*key)" src/praisonai/praisonai/cli/configuration/loader.pyRepository: MervinPraison/PraisonAI
Length of output: 13573
🏁 Script executed:
#!/bin/bash
set -e
# Inspect full example to understand structure and what config keys it uses
sed -n '1,120p' examples/yaml/agent-with-mcp.yaml
# Search for YAML parsing/loader in repo
rg -n --hidden --no-ignore-vcs "import yaml|yaml\.safe_load|from_yaml|ruamel\.yaml|\.yml|\.yaml" src/praisonai/praisonai -S
# Search for where "mcp:" is interpreted (YAML or agent config)
rg -n --hidden --no-ignore-vcs "mcp:\s*|mcp\.servers|mcp\s*=" examples src/praisonai/praisonai -SRepository: MervinPraison/PraisonAI
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
# Find how YAML "mcp" is mapped to python fields
rg -n --hidden --no-ignore-vcs "['\"]mcp['\"]\s*:|mcp\.servers|servers.*mcp|yaml_to_python_map.*mcp|\"mcp\"" src/praisonai/praisonai/cli/unified_schema.py src/praisonai/praisonai/cli/fallback_schema.py
# Find where YAML mcp config is consumed (agents/workflows YAML parsing path)
rg -n --hidden --no-ignore-vcs "mcp\.servers|config\.get\(['\"]mcp['\"]\)|\"mcp\" in |yaml.*mcp|safe_load\(.*\).*mcp" src/praisonai/praisonai | head -n 200
# Narrow down to agent YAML loader / scheduler
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/scheduler/agent_scheduler.py src/praisonai/praisonai/scheduler/yaml_loader.py src/praisonai/praisonai/recipe/core.py src/praisonai/praisonai/recipe/policy.py src/praisonai/praisonai/recipe/runtime.pyRepository: MervinPraison/PraisonAI
Length of output: 49
🏁 Script executed:
#!/bin/bash
set +e
echo "== unified_schema mcp mappings =="
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/cli/unified_schema.py | head -n 200
echo "== fallback_schema mcp mappings =="
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/cli/fallback_schema.py | head -n 200
echo "== YAML agent loader: agent_scheduler/yaml_loader =="
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/scheduler/agent_scheduler.py src/praisonai/praisonai/scheduler/yaml_loader.py | head -n 200
echo "== recipe/runtime policy: mcp usage =="
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/recipe | head -n 200
echo "== CLI main: where mcp flag/config applied =="
rg -n --hidden --no-ignore-vcs "mcp" src/praisonai/praisonai/cli/main.py | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 5568
🏁 Script executed:
#!/bin/bash
set -e
echo "== rg mcp in scheduler =="
rg -n --hidden --no-ignore-vcs "mcp:" src/praisonai/praisonai/scheduler | head -n 200
echo "== rg mcp in scheduler/yaml_loader.py =="
rg -n --hidden --no-ignore-vcs "\bmcp\b" src/praisonai/praisonai/scheduler/yaml_loader.py | head -n 200
echo "== rg mcp in scheduler/agent_scheduler.py =="
rg -n --hidden --no-ignore-vcs "\bmcp\b" src/praisonai/praisonai/scheduler/agent_scheduler.py | head -n 200
echo "== rg mcp in recipe =="
rg -n --hidden --no-ignore-vcs "\bmcp\b" src/praisonai/praisonai/recipe | head -n 200
echo "== inspect YAML loader functions around config creation =="
sed -n '300,520p' src/praisonai/praisonai/scheduler/yaml_loader.py | cat -nRepository: MervinPraison/PraisonAI
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -e
# Search for how YAML configs access an "mcp" block
rg -n --hidden --no-ignore-vcs 'get\(\s*["'\'']mcp["'\'']\s*\)|\[\s*["'\'']mcp["'\'']\s*\]|["'\'']mcp["'\'']\s*:' src/praisonai/praisonai | head -n 200
# Search for how "mcp.servers" is referenced
rg -n --hidden --no-ignore-vcs 'mcp\.servers' src/praisonai/praisonai | head -n 200
# Search for "MCP" (caps) in YAML/schema mapping code
rg -n --hidden --no-ignore-vcs '\bMCP\b|MCPLocalConfig|MCPRemoteConfig' src/praisonai/praisonai/cli/unified_schema.py src/praisonai/praisonai/cli/fallback_schema.py src/praisonai/praisonai/scheduler src/praisonai/praisonai/recipe | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 2834
🏁 Script executed:
#!/bin/bash
set -e
# Inspect likely filtering code in mcp_server/server.py around the reference
sed -n '200,340p' src/praisonai/praisonai/mcp_server/server.py | cat -n
# Search for include/exclude usage in MCP server components
rg -n --hidden --no-ignore-vcs "\binclude\b|\bexclude\b|allowed_tools|disabled_tools|tools\.include|tools\.exclude" src/praisonai/praisonai/mcp_server src/praisonai/praisonai/cli/features/mcp.py src/praisonai/praisonai/cli/commands/mcp.py | head -n 200
# Search for configuration-driven filtering patterns (tools allow/deny) in MCP related code
rg -n --hidden --no-ignore-vcs "tools.*(allow|deny|include|exclude)|allowed_tools|disabled_tools" src/praisonai/praisonai | head -n 200Repository: MervinPraison/PraisonAI
Length of output: 11611
Document tool include/exclude is unsupported for MCP servers
mcp.servers.<name>.tools.include / tools.exclude are not supported by the MCP server config schema (MCPLocalConfig/MCPRemoteConfig have no tools/include/exclude fields, and ConfigSchema.from_dict() ignores unknown server keys). praisonai mcp sync also fetches all tools via mcp.get_tools() with no filtering.
Remove the commented example or clearly label it as unsupported/inapplicable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/yaml/agent-with-mcp.yaml` around lines 38 - 47, The commented
example showing mcp.servers.<name>.tools.include and tools.exclude is misleading
because the MCP server config types (MCPLocalConfig / MCPRemoteConfig) and
ConfigSchema.from_dict() do not support a tools/include/exclude field and sync
uses mcp.get_tools() to fetch all tools; either remove this commented block
entirely or replace it with a clear note stating that per-server tool
include/exclude is unsupported for MCP servers and instruct users to filter
tools elsewhere (e.g., after mcp.get_tools()), referencing the mcp.servers,
tools.include/tools.exclude symbols and the
MCPLocalConfig/MCPRemoteConfig/ConfigSchema.from_dict()/mcp.get_tools() APIs so
maintainers can find and update the snippet.
| # Apply include filter first (takes precedence) | ||
| if self.allowed_tools: | ||
| tool_defs = filter_tools_by_allowlist(tool_defs, self.allowed_tools) | ||
|
|
||
| # Apply exclude filter | ||
| if self.disabled_tools: | ||
| tool_defs = filter_disabled_tools(tool_defs, self.disabled_tools) |
There was a problem hiding this comment.
Honor the documented allow-over-disable precedence.
With the current order, a tool present in both allowed_tools and disabled_tools is still removed. That contradicts the constructor note and the new API contract.
Proposed fix
- # Apply include filter first (takes precedence)
- if self.allowed_tools:
- tool_defs = filter_tools_by_allowlist(tool_defs, self.allowed_tools)
-
- # Apply exclude filter
- if self.disabled_tools:
- tool_defs = filter_disabled_tools(tool_defs, self.disabled_tools)
+ # Include wins over exclude.
+ if self.allowed_tools:
+ tool_defs = filter_tools_by_allowlist(tool_defs, self.allowed_tools)
+ elif self.disabled_tools:
+ tool_defs = filter_disabled_tools(tool_defs, self.disabled_tools)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Apply include filter first (takes precedence) | |
| if self.allowed_tools: | |
| tool_defs = filter_tools_by_allowlist(tool_defs, self.allowed_tools) | |
| # Apply exclude filter | |
| if self.disabled_tools: | |
| tool_defs = filter_disabled_tools(tool_defs, self.disabled_tools) | |
| # Include wins over exclude. | |
| if self.allowed_tools: | |
| tool_defs = filter_tools_by_allowlist(tool_defs, self.allowed_tools) | |
| elif self.disabled_tools: | |
| tool_defs = filter_disabled_tools(tool_defs, self.disabled_tools) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py` around lines 529 - 535, The
current filtering order removes tools present in both allowed_tools and
disabled_tools contrary to the documented precedence; change the order so that
filter_disabled_tools is applied first and then filter_tools_by_allowlist is
applied (i.e., call filter_disabled_tools(tool_defs, self.disabled_tools) before
calling filter_tools_by_allowlist(tool_defs, self.allowed_tools)) so that
allowed_tools wins over disabled_tools; update the logic around the symbols
tool_defs, allowed_tools, disabled_tools, filter_disabled_tools and
filter_tools_by_allowlist in the mcp.MCP class accordingly.
| if platform.system() == 'Windows': | ||
| safe_baseline.update({ | ||
| 'USERNAME': os.environ.get('USERNAME', ''), | ||
| 'USERPROFILE': os.environ.get('USERPROFILE', ''), | ||
| 'APPDATA': os.environ.get('APPDATA', ''), | ||
| 'LOCALAPPDATA': os.environ.get('LOCALAPPDATA', ''), | ||
| 'TEMP': os.environ.get('TEMP', ''), | ||
| 'TMP': os.environ.get('TMP', ''), | ||
| }) |
There was a problem hiding this comment.
Keep SYSTEMROOT in the Windows baseline.
This environment is built from scratch. On Windows, omitting SYSTEMROOT can prevent Python and child executables from starting correctly, so stdio MCP servers may fail only on that platform.
Proposed fix
if platform.system() == 'Windows':
safe_baseline.update({
+ 'SYSTEMROOT': os.environ.get('SYSTEMROOT', ''),
+ 'COMSPEC': os.environ.get('COMSPEC', ''),
'USERNAME': os.environ.get('USERNAME', ''),
'USERPROFILE': os.environ.get('USERPROFILE', ''),
'APPDATA': os.environ.get('APPDATA', ''),
'LOCALAPPDATA': os.environ.get('LOCALAPPDATA', ''),
'TEMP': os.environ.get('TEMP', ''),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py` around lines 571 - 579, The
Windows-specific safe_baseline update in mcp.py omits SYSTEMROOT, which can
break Python/child executables; modify the if block that updates safe_baseline
(the dict inside the platform.system() == 'Windows' branch) to include
'SYSTEMROOT': os.environ.get('SYSTEMROOT', '') so the environment baseline
retains SYSTEMROOT for stdio MCP servers.
| def list_tools(self) -> List[Dict[str, Any]]: | ||
| """ | ||
| List available tools from the MCP server. | ||
|
|
||
| Returns: | ||
| List of tool definitions from the server | ||
| """ | ||
| ... | ||
|
|
||
| def get_tools(self) -> List[Dict[str, Any]]: | ||
| """ | ||
| Get available tools from the MCP server. | ||
|
|
||
| Alias for list_tools() to maintain backward compatibility. | ||
|
|
||
| Returns: | ||
| List of tool definitions from the server | ||
| """ | ||
| ... | ||
|
|
||
| def call_tool(self, name: str, args: Dict[str, Any]) -> Any: | ||
| """ | ||
| Call a tool on the MCP server. | ||
|
|
||
| Args: | ||
| name: Tool name to call | ||
| args: Tool arguments | ||
|
|
||
| Returns: | ||
| Tool execution result | ||
|
|
||
| Raises: | ||
| Exception: If tool call fails | ||
| """ | ||
| ... | ||
|
|
||
| def shutdown(self) -> None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add async methods to the client contract.
This protocol only defines blocking I/O. In the core SDK that forces adapters to invent their own async surface or call sync MCP operations from async code. Please add async counterparts for tool listing, invocation, and shutdown so every implementation exposes the same non-blocking contract.
As per coding guidelines, "All I/O operations must have both sync and async variants; never block the event loop with sync I/O in async context; use asyncio primitives for coordination, not threading".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/mcp/protocols.py` around lines 22 - 58,
The protocol currently only exposes blocking methods (list_tools, get_tools,
call_tool, shutdown); add async counterparts on the same protocol class so
implementations can provide non-blocking I/O: define async_list_tools (async def
returning List[Dict[str, Any]]), async_get_tools (async alias to
async_list_tools), async_call_tool (async def taking name: str, args: Dict[str,
Any] and returning Any, raising on failure), and async_shutdown (async def
returning None); ensure signatures and docstrings mirror their sync equivalents
and use asyncio-friendly return types so adapters can implement both sync and
async variants consistently.
| """ | ||
| Unit tests for MCP loader functionality. | ||
|
|
||
| Tests the smoke functionality for the new MCP client lifecycle | ||
| features implemented in this issue. | ||
| """ | ||
|
|
||
| import pytest | ||
| from unittest.mock import Mock, MagicMock | ||
| from praisonaiagents.memory.mcp_config import MCPConfig | ||
|
|
||
|
|
||
| def test_mcp_config_to_mcp_instance_import_fix(): | ||
| """Test that MCPConfig.to_mcp_instance() imports MCP correctly (A1).""" | ||
| # Create a minimal config | ||
| config = MCPConfig( | ||
| name="test", | ||
| command="echo", | ||
| args=["hello"], | ||
| enabled=True | ||
| ) | ||
|
|
||
| # This should not raise ImportError anymore | ||
| # Note: It might return None if MCP package is not installed, but the import should work | ||
| try: | ||
| result = config.to_mcp_instance() | ||
| # If we get here without ImportError from wrong path, the fix worked | ||
| assert True | ||
| except ImportError as e: | ||
| if "praisonaiagents.tools.mcp" in str(e): | ||
| pytest.fail("Import still using wrong path - A1 fix failed") | ||
| else: | ||
| # This is expected if mcp package is not installed | ||
| assert "praisonaiagents[mcp]" in str(e) | ||
|
|
||
|
|
||
| def test_load_mcp_tools_import(): | ||
| """Test that load_mcp_tools can be imported (B2).""" | ||
| from praisonaiagents.mcp import load_mcp_tools | ||
| assert callable(load_mcp_tools) | ||
|
|
||
|
|
||
| def test_mcp_client_protocol_import(): | ||
| """Test that MCPClientProtocol can be imported (B1).""" | ||
| from praisonaiagents.mcp import MCPClientProtocol | ||
| # Should be a protocol/type | ||
| assert MCPClientProtocol is not None | ||
|
|
||
|
|
||
| def test_mcp_filter_parameters(): | ||
| """Test that MCP class accepts filter parameters (B3).""" | ||
| try: | ||
| from praisonaiagents.mcp import MCP | ||
| # This should not raise TypeError for unknown parameters | ||
| # Note: May raise ImportError if mcp package not installed | ||
| mcp = MCP("echo hello", allowed_tools=["test"], disabled_tools=["bad"]) | ||
| assert mcp.allowed_tools == ["test"] | ||
| assert mcp.disabled_tools == ["bad"] | ||
| except ImportError as e: | ||
| if "praisonaiagents[mcp]" in str(e): | ||
| # Expected if MCP package not installed | ||
| pytest.skip("MCP package not installed") | ||
| else: | ||
| raise | ||
|
|
||
|
|
||
| def test_filter_tool_functions_exist(): | ||
| """Test that filter functions exist and work (B3).""" | ||
| from praisonaiagents.mcp.mcp_utils import filter_tools_by_allowlist, filter_disabled_tools | ||
|
|
||
| tools = [ | ||
| {"name": "tool1", "description": "First tool"}, | ||
| {"name": "tool2", "description": "Second tool"}, | ||
| {"name": "tool3", "description": "Third tool"}, | ||
| ] | ||
|
|
||
| # Test allowlist filtering | ||
| filtered = filter_tools_by_allowlist(tools, ["tool1", "tool3"]) | ||
| assert len(filtered) == 2 | ||
| assert filtered[0]["name"] == "tool1" | ||
| assert filtered[1]["name"] == "tool3" | ||
|
|
||
| # Test disabled filtering | ||
| filtered = filter_disabled_tools(tools, ["tool2"]) | ||
| assert len(filtered) == 2 | ||
| assert all(t["name"] != "tool2" for t in filtered) | ||
|
|
||
| # Test precedence: include wins over exclude | ||
| # This is handled at MCP class level, but filters should work independently | ||
|
|
||
|
|
||
| def test_prefix_collision_handling(): | ||
| """Test tool name prefix handling (B4).""" | ||
| # The prefix logic is in the loader | ||
| from praisonaiagents.mcp.loader import load_mcp_tools | ||
|
|
||
| # Test that sanitization logic exists (basic check) | ||
| # Full test would require mock MCP configs | ||
| assert callable(load_mcp_tools) | ||
|
|
||
|
|
||
| def test_safe_env_build(): | ||
| """Test safe environment building (B5).""" | ||
| try: | ||
| from praisonaiagents.mcp import MCP | ||
|
|
||
| # Create MCP instance to test env building | ||
| # This tests that _build_safe_env method exists | ||
| mcp = MCP("echo test") | ||
| assert hasattr(mcp, '_build_safe_env') | ||
|
|
||
| # Test safe env function | ||
| safe_env = mcp._build_safe_env({"CUSTOM_VAR": "test"}) | ||
| assert isinstance(safe_env, dict) | ||
| assert "CUSTOM_VAR" in safe_env | ||
| assert safe_env["CUSTOM_VAR"] == "test" | ||
| # Should have safe baseline | ||
| assert "PATH" in safe_env | ||
| assert "HOME" in safe_env | ||
|
|
||
| except ImportError as e: | ||
| if "praisonaiagents[mcp]" in str(e): | ||
| pytest.skip("MCP package not installed") | ||
| else: | ||
| raise No newline at end of file |
There was a problem hiding this comment.
Critical: Missing mandatory agentic tests.
This test file contains only smoke tests (import checks, parameter acceptance, helper function validation) but no agentic tests. As per coding guidelines, "Real agentic tests are MANDATORY for every feature: Agent must call agent.start() with a real prompt, call the LLM, and produce actual text response—not just smoke tests of object construction."
While unit tests are valuable, the MCP loader feature requires at least one agentic test that:
- Creates an Agent with
tools=load_mcp_tools(...) - Calls
agent.start("some prompt requiring MCP tools") - Verifies the LLM produces a text response
As per coding guidelines: tests/**/*.py: Real agentic tests are MANDATORY for every feature: Agent must call agent.start() with a real prompt, call the LLM, and produce actual text response—not just smoke tests of object construction
🧪 Example agentic test structure
Add a test like this (possibly in tests/integration/ or tests/e2e/):
def test_agent_with_mcp_tools_end_to_end():
"""Real agentic test: Agent uses MCP tools to respond to prompt."""
from praisonaiagents import Agent
from praisonaiagents.mcp import load_mcp_tools
# Load MCP tools (may need mock configs for test)
tools = load_mcp_tools(names=["test_server"])
# Create agent with MCP tools
agent = Agent(
name="test_assistant",
instructions="Use available tools to help the user",
llm="gpt-4o-mini",
tools=tools
)
# Call agent.start() with real prompt
response = agent.start("List files in current directory")
# Verify actual text response from LLM
assert response is not None
assert len(response) > 0
assert isinstance(response, str)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/tests/unit/test_mcp_loader.py` around lines 1 - 125, The
test file lacks a mandatory agentic end-to-end test: add a new test function
(e.g., test_agent_with_mcp_tools_end_to_end) that imports load_mcp_tools and
Agent, obtains tools via load_mcp_tools(...), constructs an Agent instance
(Agent(..., tools=tools)), calls agent.start("a prompt that exercises MCP
tools"), and asserts the returned response is a non-empty string
(isinstance(response, str) and len(response) > 0); ensure the test handles
ImportError/skips if MCP package or LLM is unavailable but otherwise runs a real
agentic call using the Agent.start method.
| if response.status_code == 200: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "ok", "message": "Remote server reachable"}) | ||
| else: | ||
| output.print_success(f"Remote server '{name}' is reachable") | ||
| else: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "error", "message": f"HTTP {response.status_code}"}) | ||
| else: | ||
| output.print_error(f"Remote server returned HTTP {response.status_code}") | ||
| raise typer.Exit(1) | ||
| except ImportError: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "ok", "message": "Server started successfully"}) | ||
| output.print_json({"name": name, "status": "error", "message": "httpx not available for remote testing"}) | ||
| else: | ||
| output.print_success(f"Server '{name}' started successfully") | ||
| else: | ||
| # Process exited | ||
| stderr = proc.stderr.read().decode() if proc.stderr else "" | ||
| output.print_error("httpx package not available. Install with: pip install httpx") | ||
| raise typer.Exit(1) | ||
| except Exception as e: | ||
| if output.is_json_mode: | ||
| output.print_json({"name": name, "status": "error", "message": stderr or "Server exited immediately"}) | ||
| output.print_json({"name": name, "status": "error", "message": str(e)}) | ||
| else: | ||
| output.print_error(f"Server exited immediately: {stderr}") | ||
| output.print_error(f"Failed to connect to remote server: {e}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is typer.Exit a subclass of Exception in typer 0.9.0?
💡 Result:
No. In Typer 0.9.0, typer.Exit is a subclass of click.exceptions.Exit (which itself subclasses RuntimeError), not of Exception. Typer’s docs describe using typer.Exit by raising it to terminate a command [1]. Typer implements “Exit” by re-exporting/using Click’s Exit exception (which is defined as class Exit(RuntimeError)) [2], so it’s not a direct subclass of Exception in the sense of inheriting directly from Exception; it inherits from RuntimeError instead.
Citations:
- 1: https://typer.tiangolo.com/tutorial/terminating/
- 2: https://github.com/pallets/click/blob/master/src/click/exceptions.py
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import inspect
try:
import typer, click
print("typer", getattr(typer, "__version__", "unknown"))
print("click", getattr(click, "__version__", "unknown"))
Exit = typer.Exit
print("typer.Exit bases:", Exit.__bases__)
print("typer.Exit mro:", Exit.mro())
print("Is typer.Exit subclass of Exception?", issubclass(Exit, Exception))
from click.exceptions import Exit as ClickExit
print("click.exceptions.Exit mro:", ClickExit.mro())
except Exception as e:
print("Could not import typer/click in sandbox:", repr(e))
PYRepository: MervinPraison/PraisonAI
Length of output: 472
Handle typer.Exit separately so it doesn’t get caught by the generic except Exception.
typer.Exit is an Exception subclass, so the raise typer.Exit(1) on HTTP non-200 (non-JSON path) is caught by except Exception as e, which then prints Failed to connect to remote server: 1 in addition to the earlier HTTP error.
🐛 Let Exit propagate
except ImportError:
...
+ except typer.Exit:
+ raise
except Exception as e:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if response.status_code == 200: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "ok", "message": "Remote server reachable"}) | |
| else: | |
| output.print_success(f"Remote server '{name}' is reachable") | |
| else: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "error", "message": f"HTTP {response.status_code}"}) | |
| else: | |
| output.print_error(f"Remote server returned HTTP {response.status_code}") | |
| raise typer.Exit(1) | |
| except ImportError: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "ok", "message": "Server started successfully"}) | |
| output.print_json({"name": name, "status": "error", "message": "httpx not available for remote testing"}) | |
| else: | |
| output.print_success(f"Server '{name}' started successfully") | |
| else: | |
| # Process exited | |
| stderr = proc.stderr.read().decode() if proc.stderr else "" | |
| output.print_error("httpx package not available. Install with: pip install httpx") | |
| raise typer.Exit(1) | |
| except Exception as e: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "error", "message": stderr or "Server exited immediately"}) | |
| output.print_json({"name": name, "status": "error", "message": str(e)}) | |
| else: | |
| output.print_error(f"Server exited immediately: {stderr}") | |
| output.print_error(f"Failed to connect to remote server: {e}") | |
| raise typer.Exit(1) | |
| if response.status_code == 200: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "ok", "message": "Remote server reachable"}) | |
| else: | |
| output.print_success(f"Remote server '{name}' is reachable") | |
| else: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "error", "message": f"HTTP {response.status_code}"}) | |
| else: | |
| output.print_error(f"Remote server returned HTTP {response.status_code}") | |
| raise typer.Exit(1) | |
| except ImportError: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "error", "message": "httpx not available for remote testing"}) | |
| else: | |
| output.print_error("httpx package not available. Install with: pip install httpx") | |
| raise typer.Exit(1) | |
| except typer.Exit: | |
| raise | |
| except Exception as e: | |
| if output.is_json_mode: | |
| output.print_json({"name": name, "status": "error", "message": str(e)}) | |
| else: | |
| output.print_error(f"Failed to connect to remote server: {e}") | |
| raise typer.Exit(1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/commands/mcp.py` around lines 178 - 200, The
generic except block is catching the explicit raise typer.Exit(1) from the HTTP
non-200 path; update the exception handlers in the function that performs the
remote check so typer.Exit is allowed to propagate: add a specific "except
typer.Exit: raise" clause before the broad "except Exception as e" (or,
alternatively, inside the generic handler re-raise when isinstance(e,
typer.Exit)); keep the existing json/non-json logging in the other handlers and
only handle non-typer exceptions in the final except block.
| env = dict(**server.env) if server.env else None | ||
|
|
||
| # Start process | ||
| proc = subprocess.Popen( | ||
| cmd, | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| env=env, | ||
| ) |
There was a problem hiding this comment.
Local test drops the parent environment when server.env is set.
env = dict(**server.env) if server.env else None passes only the configured vars to subprocess.Popen, which replaces (not merges) the process environment. A server whose command is e.g. npx will then lose PATH and fail with FileNotFoundError, so any server defining custom env can never pass mcp test. Merge onto a baseline env instead.
🐛 Merge configured env onto the inherited environment
- cmd = [server.command] + server.args
- env = dict(**server.env) if server.env else None
+ cmd = [server.command] + server.args
+ env = None
+ if server.env:
+ import os
+ env = {**os.environ, **server.env}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/commands/mcp.py` around lines 212 - 221, The code
currently replaces the child process environment when server.env is set (env =
dict(**server.env)), causing inherited vars like PATH to be lost; modify the
logic before subprocess.Popen to create a merged environment by copying the
current process environment (os.environ) and updating it with server.env so that
server.env augments/overrides defaults rather than replaces them, then pass that
merged env into the subprocess.Popen call that uses cmd, stdin/stdout/stderr and
env.
| locations = [ | ||
| Path.cwd() / ".praison" / "mcp.json", | ||
| Path.cwd() / ".praison" / "mcp" / "config.json", | ||
| Path.home() / ".praison" / "mcp.json", | ||
| Path.home() / ".config" / "praison" / "mcp.json", | ||
| ] |
There was a problem hiding this comment.
Add canonical project path to fallback branch for consistency.
The fallback branch (when praisonaiagents is not available) only includes legacy .praison paths. It's missing the canonical project-level path Path.cwd() / ".praisonai" / "mcp" / "config.json", which is included in the main branch (line 30 via project_mcp_dir).
This creates an inconsistency: if a user creates a project-level MCP config at .praisonai/mcp/config.json and praisonaiagents is not installed, the config won't be discovered.
📂 Proposed fix to add canonical project path
except ImportError:
# Fallback to legacy paths if praisonaiagents not available
locations = [
+ Path.cwd() / ".praisonai" / "mcp" / "config.json",
Path.cwd() / ".praison" / "mcp.json",
Path.cwd() / ".praison" / "mcp" / "config.json",
Path.home() / ".praison" / "mcp.json",
Path.home() / ".config" / "praison" / "mcp.json",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| locations = [ | |
| Path.cwd() / ".praison" / "mcp.json", | |
| Path.cwd() / ".praison" / "mcp" / "config.json", | |
| Path.home() / ".praison" / "mcp.json", | |
| Path.home() / ".config" / "praison" / "mcp.json", | |
| ] | |
| locations = [ | |
| Path.cwd() / ".praisonai" / "mcp" / "config.json", | |
| Path.cwd() / ".praison" / "mcp.json", | |
| Path.cwd() / ".praison" / "mcp" / "config.json", | |
| Path.home() / ".praison" / "mcp.json", | |
| Path.home() / ".config" / "praison" / "mcp.json", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/cli/features/doctor/checks/mcp_checks.py` around
lines 40 - 45, The fallback locations list in the branch used when
praisonaiagents is not available (the variable named locations) omits the
canonical project-level MCP path; add Path.cwd() / ".praisonai" / "mcp" /
"config.json" to that locations list so it matches the main branch's
project_mcp_dir discovery logic (keep the rest of the legacy `.praison`
fallbacks intact).
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job I'll analyze this and get back to you. |
1 participant
Summary
Implements comprehensive fixes for MCP client lifecycle addressing broken imports, misleading OAuth documentation, and missing remote server support. Adds thin wiring between config and agents following protocol-driven architecture.
Resolves #1776
Changes
Tier A - Critical Fixes
MCPConfig.to_mcp_instance()broken import frompraisonaiagents.mcp~/.praisonai/mcp/pathsTier B - Core Agent Value
MCPClientProtocolinterface for extensibilityload_mcp_tools()helper for agent-centric tool loadingallowed_tools/disabled_toolsparametersTier C - Wrapper Improvements
mcp test/mcp syncCLI commandsTesting
Architecture Compliance
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests