feat(core): Modernize Dependency Management with Auto-UVX #284
Conversation
- Remove unused 'is_non_interactive' import in install.py - Use stderr for installation status message in install.py
- Upgrades execution to 'uv run' if uv is detected - Solves dependency isolation and Pydantic version conflicts - Eliminates need for pip install step for Python servers
WalkthroughAdds non-interactive handling across install/uninstall, auto-wraps Python/pip installs with UVX when available, expands server types and transport support, removes legacy v1 migration flow replacing it with a transport-only migrator, adds client config path getters/setters and a client config CLI, and introduces proxy and doctor health checks plus related tests and metadata updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mcpm/commands/install.py (2)
124-126: Passforceto recursive call for consistency.The recursive call omits the
forceparameter. While currently unreachable in non-interactive mode (the function returns early), passing it maintains consistency and prevents subtle bugs if the logic changes.if not result.strip() and required and not default: console.print("[yellow]Warning: Required value cannot be empty.[/]") - return prompt_with_default(prompt_text, default, hide_input, required) + return prompt_with_default(prompt_text, default, hide_input, required, force)
446-451: Remove or implement the-mheuristic block.This block contains only a
passstatement and extensive comments describing a heuristic that's never executed. Either implement the module-to-package name inference or remove this dead code to avoid confusion.- # If args start with '-m', the next arg is the module. - # Often module == package (e.g. mcp_server_time -> mcp-server-time? No, dashes vs underscores). - # But 'uv run --with <module> python -m <module>' usually works if PyPI name matches. - - if not target_package and mcp_args and mcp_args[0] == "-m" and len(mcp_args) > 1: - # Heuristic: Assume package name matches module name (with _ -> - maybe?) - # Ideally, the registry should provide 'package'. - # For now, we only auto-upgrade if we have a package name OR if we are brave. - # Let's rely on package_name variable extracted earlier from selected_method.get("package") - pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcpm/commands/install.py(8 hunks)src/mcpm/commands/uninstall.py(2 hunks)src/mcpm/utils/non_interactive.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
src/mcpm/commands/install.pysrc/mcpm/utils/non_interactive.pysrc/mcpm/commands/uninstall.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: pathintegral-institute/mcpm.sh PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:20:00.958Z
Learning: Use `uv` for all Python dependency management.
🧬 Code graph analysis (2)
src/mcpm/commands/install.py (2)
src/mcpm/utils/non_interactive.py (1)
should_force_operation(35-45)src/mcpm/schemas/full_server_config.py (1)
to_server_config(72-84)
src/mcpm/commands/uninstall.py (1)
src/mcpm/utils/non_interactive.py (1)
should_force_operation(35-45)
🔇 Additional comments (4)
src/mcpm/commands/uninstall.py (1)
10-10: LGTM! Consistent non-interactive handling.The integration of
should_force_operation(force)correctly unifies force behavior with the install command, respecting both the CLI--forceflag and theMCPM_FORCEenvironment variable.Also applies to: 63-63
src/mcpm/utils/non_interactive.py (1)
35-45: LGTM! Clean extension of the force operation check.The backward-compatible signature change with a default value and the OR logic correctly enables force behavior from either CLI flags or environment variables. The case-insensitive env var check is a good practice.
src/mcpm/commands/install.py (2)
155-156: LGTM! Comprehensive non-interactive mode integration.Good implementation:
- Progress messages to stderr keeps stdout clean for structured output
- Confirmation dialogs and method selection correctly gated by
should_force_operation(force)- Force semantics properly propagated to
global_add_serverBased on learnings, the use of
uvfor Python dependency management aligns with project guidelines.Also applies to: 180-182, 227-227, 481-481
435-435: I apologize for the cloning failure. Let me reconsider based on the information available.Looking at the review comment and the code context provided, the concern about
piphandling is logically sound even without examining the registry files:Analysis of the concern:
- Line 435 checks:
if mcp_command in ["python", "python3", "pip"] and shutil.which("uv"):- The subsequent logic (lines 453-464) wraps the command:
uv run --with <package> <original_command> <args>- For
pipcommands, this would produce:uv run --with <pkg> pip install ...- This is semantically incorrect because
uv run --withis designed for running Python modules/executables, not for invoking package managersThe concern is valid because:
uv run --with <pkg>is meant to run Python code with additional dependencies, not to wrap arbitrary commandspipis a package management tool and should not be treated the same aspythonorpython3executables- If this is executed, it would either fail or produce unexpected behavior
Since I cannot access the repository to verify if
pipis actually used in registry entries, but the concern itself is technically correct, I'll preserve the original review comment with validation that the concern is sound.Reconsider including
pipin auto-UVX upgrade.Wrapping
pipcommands withuv run --with <pkg> pip ...is semantically incorrect. Theuv run --withpattern is designed for running Python executables/modules, not for package installation commands. Either excludepipfrom this check, or ifpipcommands in the registry actually represent something else (unlikely), add a clarifying comment explaining the intent.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcpm/commands/new.py (1)
27-35: Help/error text is stale for new server types.
- The “Server name is required” hint still mentions only
stdio|remote.--urlhelp should includesse/streamable-http.-@click.option("--url", help="Server URL (required for remote servers)") +@click.option("--url", help="Server URL (required for remote/sse/streamable-http servers)") @@ if not server_name: - print_error("Server name is required", "Use: mcpm new <server_name> --type <stdio|remote>") + print_error( + "Server name is required", + "Use: mcpm new <server_name> --type <stdio|remote|sse|streamable-http>", + ) return 1Also applies to: 85-93
🧹 Nitpick comments (4)
src/mcpm/core/schema.py (1)
71-75: Maketransporta constrained type (avoid silent typos); considerdefault_factoryfor mutable defaults.
transport: Optional[str]invites misspellings (“streamable_http”, etc.). Prefer a constrained type:-from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union, Literal @@ class RemoteServerConfig(BaseServerConfig): url: str - transport: Optional[str] = None # "sse" or "streamable-http" + transport: Optional[Literal["sse", "streamable-http"]] = None headers: Dict[str, Any] = {}Separate (optional but recommended): replace
=[]/={}defaults in Pydantic models withField(default_factory=...)to avoid any shared-mutable-default ambiguity.src/mcpm/clients/client_config.py (1)
25-37: Consider path normalization/validation and an “unset” flow for client paths.Optional hardening:
- Normalize
path(e.g., expanduser/resolve) before persisting.- Add
unset_client_path(client_name)(or allowset_client_path(..., path: str | None)), sinceConfigManager.set_config()already supports deleting keys when passedNone(seemcpm/utils/config.pyin provided snippets).tests/test_transport_migration.py (1)
12-96: Good coverage of core migration rules; consider asserting_save_configcall in VSCode test.
test_migrate_vscode_structureindirectly assumes_save_configis called (it readscall_args). Addingmock_manager._save_config.assert_called_once()would make failures clearer.src/mcpm/migration/transport_migrator.py (1)
15-96: Registry usage is misleading (instance stored, classmethod called) + URL rewrite is too broad.
If
ClientRegistry.get_all_client_managersis a@classmethod, storingself.registry = ClientRegistry()doesn’t buy you anything and makes tests/mock strategy less representative. Prefer either:
- call
ClientRegistry.get_all_client_managers()directly and dropself.registry, or- accept an injected registry object with an instance
get_all_client_managers()for testability.URL rewrite should avoid rewriting non-segment matches (e.g.
/ssefoo). Consider parsing and rewriting only the path segment.Printing should fall back if
display_nameis missing.Sketch of the “class-based registry” approach:
class TransportMigrator: @@ - def __init__(self): - self.registry = ClientRegistry() + def __init__(self): + pass @@ - managers = self.registry.get_all_client_managers() + managers = ClientRegistry.get_all_client_managers() @@ - console.print(f" ✅ Updated [green]{manager.display_name}[/]") + display = getattr(manager, "display_name", name) + console.print(f" ✅ Updated [green]{display}[/]")(If you take this route, adjust
tests/test_transport_migration.pyto patch the classmethod rather than replacing the class.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.gitignore(1 hunks)mcp-registry/servers/firecrawl.json(1 hunks)src/mcpm/cli.py(3 hunks)src/mcpm/clients/client_config.py(1 hunks)src/mcpm/clients/client_registry.py(1 hunks)src/mcpm/commands/client.py(8 hunks)src/mcpm/commands/doctor.py(1 hunks)src/mcpm/commands/migrate.py(1 hunks)src/mcpm/commands/new.py(4 hunks)src/mcpm/core/schema.py(1 hunks)src/mcpm/fastmcp_integration/proxy.py(1 hunks)src/mcpm/migration/__init__.py(1 hunks)src/mcpm/migration/transport_migrator.py(1 hunks)src/mcpm/migration/v1_detector.py(0 hunks)src/mcpm/migration/v1_migrator.py(0 hunks)src/mcpm/utils/non_interactive.py(5 hunks)tests/test_client.py(12 hunks)tests/test_clients/test_qwen_cli.py(4 hunks)tests/test_migration.py(0 hunks)tests/test_transport_migration.py(1 hunks)
💤 Files with no reviewable changes (3)
- tests/test_migration.py
- src/mcpm/migration/v1_detector.py
- src/mcpm/migration/v1_migrator.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_clients/test_qwen_cli.py
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcpm/utils/non_interactive.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
src/mcpm/commands/doctor.pysrc/mcpm/core/schema.pysrc/mcpm/clients/client_config.pytests/test_transport_migration.pysrc/mcpm/migration/__init__.pytests/test_client.pysrc/mcpm/migration/transport_migrator.pysrc/mcpm/commands/client.pysrc/mcpm/clients/client_registry.pysrc/mcpm/commands/migrate.pysrc/mcpm/commands/new.pysrc/mcpm/fastmcp_integration/proxy.pysrc/mcpm/cli.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: pathintegral-institute/mcpm.sh PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:20:00.958Z
Learning: Use `uv` for all Python dependency management.
🧬 Code graph analysis (9)
src/mcpm/core/schema.py (1)
src/mcpm/monitor/base.py (1)
transport(83-89)
src/mcpm/clients/client_config.py (2)
tests/conftest.py (1)
config_manager(48-69)src/mcpm/utils/config.py (1)
set_config(94-117)
tests/test_transport_migration.py (2)
src/mcpm/migration/transport_migrator.py (2)
TransportMigrator(15-97)migrate_all_clients(21-97)src/mcpm/clients/client_registry.py (1)
get_all_client_managers(83-90)
src/mcpm/migration/__init__.py (1)
src/mcpm/migration/transport_migrator.py (1)
TransportMigrator(15-97)
src/mcpm/migration/transport_migrator.py (1)
src/mcpm/clients/client_registry.py (1)
get_all_client_managers(83-90)
src/mcpm/commands/client.py (5)
src/mcpm/clients/client_registry.py (2)
ClientRegistry(30-154)get_supported_clients(147-154)src/mcpm/clients/client_config.py (3)
get_supported_clients(38-43)set_client_path(31-36)get_client_path(25-29)src/mcpm/clients/base.py (6)
add_server(63-72)add_server(263-283)add_server(536-560)remove_server(109-118)remove_server(341-361)remove_server(562-579)src/mcpm/clients/managers/fiveire.py (1)
remove_server(63-68)src/mcpm/core/schema.py (1)
STDIOServerConfig(28-68)
src/mcpm/clients/client_registry.py (1)
src/mcpm/clients/client_config.py (1)
get_client_path(25-29)
src/mcpm/commands/migrate.py (1)
src/mcpm/migration/transport_migrator.py (2)
TransportMigrator(15-97)migrate_all_clients(21-97)
src/mcpm/commands/new.py (3)
src/mcpm/utils/display.py (1)
print_error(121-130)src/mcpm/monitor/base.py (1)
transport(83-89)src/mcpm/core/schema.py (1)
RemoteServerConfig(71-91)
🔇 Additional comments (6)
tests/test_client.py (1)
418-440: Pure reflow/formatting changes; no behavioral impact.Also applies to: 470-485, 501-526, 544-565, 580-595, 609-622
src/mcpm/cli.py (1)
6-7: CWD validation + fallback to home is a solid robustness improvement.Also applies to: 36-37, 91-103
src/mcpm/migration/__init__.py (1)
5-7: Clean migration public API surface.mcp-registry/servers/firecrawl.json (1)
7-12: The GitHub URLs in the file are correct and publicly accessible. The repositoryfirecrawl/firecrawl-mcp-serverexists and is documented in official Firecrawl resources. No action needed.src/mcpm/commands/client.py (1)
267-278: Non-interactive edit flow changes look consistent.The updated signature, change summaries, and
STDIOServerConfig(...)construction for profiles/servers are straightforward and match the intent of the non-interactive path.Also applies to: 1314-1333, 1366-1387
src/mcpm/commands/new.py (1)
115-147: Transport propagation forsse/streamable-httplooks good.Setting
transport = server_typefor those remote-like modes and surfacing it in the summary is clean and matches the stated behavior.
| # Check if there's a stored custom path if no override provided | ||
| if not config_path_override: | ||
| stored_path = cls._client_config_manager.get_client_path(client_name) | ||
| if stored_path: | ||
| config_path_override = stored_path | ||
|
|
There was a problem hiding this comment.
Use is None if you want to distinguish “unset” from “intentionally empty”.
If callers might pass config_path_override="" intentionally, if not config_path_override: will override it. Safer:
- if not config_path_override:
+ if config_path_override is None:
stored_path = cls._client_config_manager.get_client_path(client_name)
if stored_path:
config_path_override = stored_path🤖 Prompt for AI Agents
In src/mcpm/clients/client_registry.py around lines 71 to 76, the code uses a
falsy check (if not config_path_override) which treats an intentionally empty
string the same as unset; change the condition to explicitly check for None (if
config_path_override is None) so that only the truly unset case triggers
fetching stored_path from _client_config_manager; if stored_path is found,
assign it to config_path_override, otherwise leave the provided value (including
empty string) untouched.
| @client.command(name="config", context_settings=dict(help_option_names=["-h", "--help"])) | ||
| @click.argument("client_name") | ||
| @click.option("--set-path", help="Set a custom configuration file path for the client") | ||
| @click.option("--get-path", is_flag=True, help="Get the currently stored configuration path") | ||
| @click.option("--clear-path", is_flag=True, help="Clear the stored custom configuration path") | ||
| def config_client(client_name, set_path, get_path, clear_path): | ||
| """Configure client settings (e.g., custom config paths).""" | ||
| if not any([set_path, get_path, clear_path]): | ||
| console.print("[yellow]No action specified. Use --set-path, --get-path, or --clear-path.[/]") | ||
| return | ||
|
|
||
| # Check if client is supported | ||
| supported_clients = ClientRegistry.get_supported_clients() | ||
| if client_name not in supported_clients: | ||
| console.print(f"[red]Error: Client '{client_name}' is not supported.[/]") | ||
| return | ||
|
|
||
| if set_path: | ||
| if client_config_manager.set_client_path(client_name, set_path): | ||
| console.print(f"[green]Successfully set custom config path for {client_name}:[/] {set_path}") | ||
| else: | ||
| console.print(f"[red]Failed to set config path for {client_name}[/]") | ||
|
|
||
| if clear_path: | ||
| if client_config_manager.set_client_path(client_name, None): | ||
| console.print(f"[green]Successfully cleared custom config path for {client_name}[/]") | ||
| else: | ||
| console.print(f"[red]Failed to clear config path for {client_name}[/]") | ||
|
|
||
| if get_path or set_path or clear_path: # Always show current path after modification or if requested | ||
| current_path = client_config_manager.get_client_path(client_name) | ||
| if current_path: | ||
| console.print(f"[bold]Current stored path for {client_name}:[/] {current_path}") | ||
| else: | ||
| console.print(f"[dim]No custom path stored for {client_name} (using default)[/]") | ||
|
|
There was a problem hiding this comment.
--clear-path likely doesn’t clear (stores null / violates set_client_path contract).
ClientConfigManager.set_client_path(self, client_name: str, path: str) -> bool (per provided snippet) doesn’t accept None, and even if it “works”, it likely persists a null entry instead of removing it.
Also, --set-path/--get-path/--clear-path can be combined with ambiguous results (e.g., set+clear).
Suggested shape:
- Make
set_client_pathacceptOptional[str]and delete the key whenNone, or add an explicitclear_client_path(client_name)API and call that here. - Enforce exactly one action flag per invocation.
def config_client(client_name, set_path, get_path, clear_path):
"""Configure client settings (e.g., custom config paths)."""
- if not any([set_path, get_path, clear_path]):
+ actions = sum(bool(x) for x in [set_path, get_path, clear_path])
+ if actions == 0:
console.print("[yellow]No action specified. Use --set-path, --get-path, or --clear-path.[/]")
return
+ if actions > 1:
+ console.print("[red]Error: choose only one of --set-path, --get-path, or --clear-path.[/]")
+ return
@@
if set_path:
+ set_path = os.path.abspath(os.path.expanduser(set_path))
if client_config_manager.set_client_path(client_name, set_path):
console.print(f"[green]Successfully set custom config path for {client_name}:[/] {set_path}")
@@
if clear_path:
- if client_config_manager.set_client_path(client_name, None):
+ # Prefer: client_config_manager.clear_client_path(client_name)
+ if client_config_manager.set_client_path(client_name, None):
console.print(f"[green]Successfully cleared custom config path for {client_name}[/]")(You’d still need the ClientConfigManager change to make the None call correct.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mcpm/commands/client.py around lines 51 to 86, the CLI currently allows
combining --set-path/--get-path/--clear-path and calls
set_client_path(client_name, None) to clear which violates the existing
set_client_path signature and may persist a null entry; enforce exactly one
action flag per invocation (error out if zero or more than one flags are
provided), and replace the call that attempts to clear by invoking an explicit
clear_client_path(client_name) method (or, if you choose to change
ClientConfigManager instead, update set_client_path to accept Optional[str] and
delete the stored key when path is None and then call it here). Ensure the code
validates flags first, then performs one of: set_client_path(client_name,
set_path), clear_client_path(client_name) (or set_client_path(client_name, None)
only after updating its signature/behavior), or get_client_path(client_name),
and only show the current path after the single successful action.
| # 8. Check Daemon Status | ||
| console.print("[bold cyan]🤖 Daemon Status[/]") | ||
| try: | ||
| import urllib.request | ||
|
|
||
| try: | ||
| with urllib.request.urlopen("http://localhost:6276/health", timeout=2) as response: | ||
| if response.status == 200: | ||
| console.print(" ✅ MCPM Daemon is running (port 6276)") | ||
| else: | ||
| console.print(f" ⚠️ MCPM Daemon responded with status {response.status}") | ||
| issues_found += 1 | ||
| except Exception as e: | ||
| console.print(f" ❌ MCPM Daemon not reachable: {e}") | ||
| console.print(" (Run 'mcpm-daemon' or 'docker compose up' to start it)") | ||
| issues_found += 1 | ||
| except Exception as e: | ||
| console.print(f" ❌ Error checking daemon: {e}") | ||
| issues_found += 1 | ||
|
|
There was a problem hiding this comment.
Fix non-200 handling: urlopen() raises HTTPError, so the response.status != 200 branch likely won’t run.
Right now, a non-200 health response will typically be reported as “not reachable” (via the broad exception) instead of “responded with status X”, which makes the diagnostic misleading.
# 8. Check Daemon Status
console.print("[bold cyan]🤖 Daemon Status[/]")
try:
import urllib.request
+ import urllib.error
try:
with urllib.request.urlopen("http://localhost:6276/health", timeout=2) as response:
if response.status == 200:
console.print(" ✅ MCPM Daemon is running (port 6276)")
else:
console.print(f" ⚠️ MCPM Daemon responded with status {response.status}")
issues_found += 1
- except Exception as e:
- console.print(f" ❌ MCPM Daemon not reachable: {e}")
+ except urllib.error.HTTPError as e:
+ console.print(f" ⚠️ MCPM Daemon responded with status {e.code}")
+ issues_found += 1
+ except (urllib.error.URLError, TimeoutError) as e:
+ console.print(f" ❌ MCPM Daemon not reachable: {e}")
console.print(" (Run 'mcpm-daemon' or 'docker compose up' to start it)")
issues_found += 1
except Exception as e:
console.print(f" ❌ Error checking daemon: {e}")
issues_found += 1Optional: consider whether “daemon not running” should always increment issues_found (vs. only when daemon mode is configured/enabled).
🤖 Prompt for AI Agents
In src/mcpm/commands/doctor.py around lines 173 to 192, urllib.request.urlopen
raises urllib.error.HTTPError for non-200 responses so the current broad except
hides status codes; change the try/except to explicitly catch HTTPError and
URLError (or import urllib.error.HTTPError and URLError), on HTTPError read the
.code and print "MCPM Daemon responded with status X" and increment
issues_found, on URLError or other exceptions print "Daemon not reachable" and
increment issues_found; keep success branch for a 200 response unchanged.
| @click.command() | ||
| @click.option("--force", is_flag=True, help="Force migration even if v1 config not detected") | ||
| @click.option("--force", is_flag=True, help="Force migration checks") | ||
| @click.help_option("-h", "--help") | ||
| def migrate(force): |
There was a problem hiding this comment.
--force is unused (misleading CLI flag).
migrate(force) never references force, so users can’t “force migration checks” as help text claims.
@click.command()
-@click.option("--force", is_flag=True, help="Force migration checks")
@click.help_option("-h", "--help")
-def migrate(force):
+def migrate():
"""Migrate configuration to latest standards.
@@
migrator = TransportMigrator()
if migrator.migrate_all_clients():
console.print("\n[bold green]✅ Transport migration completed successfully![/]")If you intended a real “force” behavior, consider adding a force: bool = False parameter to TransportMigrator.migrate_all_clients(...) and wiring it through instead.
Also applies to: 23-32
🤖 Prompt for AI Agents
In src/mcpm/commands/migrate.py around lines 11 to 14 (and similarly for the
call sites at 23 to 32), the CLI option --force is accepted but never used; wire
it through to the migrator by adding a force: bool = False parameter to
TransportMigrator.migrate_all_clients(...) (default False), update the method
implementation to honor the flag (e.g., bypass or relax migration checks when
force is True), and pass the CLI force value into that call from migrate(force);
also update any other migrate_all_clients call sites mentioned (lines 23–32) to
pass the force argument so the CLI flag has an effect.
| # Add health check endpoint for Docker/System checks | ||
| @proxy.custom_route("/health", methods=["GET"]) | ||
| async def health_check(request): | ||
| from starlette.responses import JSONResponse | ||
|
|
||
| return JSONResponse({"status": "healthy", "service": name or "mcpm-aggregated"}) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "src/mcpm/fastmcp_integration/" | head -20Repository: pathintegral-institute/mcpm.sh
Length of output: 234
🏁 Script executed:
rg "MCPMAuthMiddleware|custom_route" src/mcpm/fastmcp_integration/ -A 5 -B 2Repository: pathintegral-institute/mcpm.sh
Length of output: 2452
🏁 Script executed:
rg "auth_enabled|stdio_mode" src/mcpm/fastmcp_integration/proxy.py -B 3 -A 3Repository: pathintegral-institute/mcpm.sh
Length of output: 4031
🏁 Script executed:
cat -n src/mcpm/fastmcp_integration/proxy.py | sed -n '100,150p'Repository: pathintegral-institute/mcpm.sh
Length of output: 2592
🏁 Script executed:
rg "class MCPMAuthMiddleware" src/mcpm/fastmcp_integration/middleware.py -A 50Repository: pathintegral-institute/mcpm.sh
Length of output: 2269
🏁 Script executed:
grep -n "import.*JSONResponse\|from starlette" src/mcpm/fastmcp_integration/proxy.pyRepository: pathintegral-institute/mcpm.sh
Length of output: 134
🏁 Script executed:
grep -n "^from\|^import" src/mcpm/fastmcp_integration/proxy.py | head -30Repository: pathintegral-institute/mcpm.sh
Length of output: 508
🏁 Script executed:
# Check if there's a way to skip auth for specific routes in FastMCP documentation or examples
rg "custom_route" src/mcpm/fastmcp_integration/proxy.py -B 5 -A 10Repository: pathintegral-institute/mcpm.sh
Length of output: 836
🏁 Script executed:
# Check the full proxy.py for import statements and structure
head -50 src/mcpm/fastmcp_integration/proxy.pyRepository: pathintegral-institute/mcpm.sh
Length of output: 1605
🏁 Script executed:
# Check if there are any conditional checks in middleware that might skip certain paths
rg "path|route|url" src/mcpm/fastmcp_integration/middleware.py -iRepository: pathintegral-institute/mcpm.sh
Length of output: 180
🏁 Script executed:
# Let's verify the exact structure of the on_request method and see if there's a call_next pattern
sed -n '1,100p' src/mcpm/fastmcp_integration/middleware.py | tail -60Repository: pathintegral-institute/mcpm.sh
Length of output: 2439
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to auth and health in the codebase
rg "TODO|FIXME" src/mcpm/fastmcp_integration/ -iRepository: pathintegral-institute/mcpm.sh
Length of output: 56
🏁 Script executed:
# Let's check if there's documentation or comments about the health endpoint design
sed -n '115,140p' src/mcpm/fastmcp_integration/proxy.pyRepository: pathintegral-institute/mcpm.sh
Length of output: 1095
🏁 Script executed:
# Check if there are any ruff/lint issues in the current code
pip install ruff -q 2>/dev/null && ruff check src/mcpm/fastmcp_integration/proxy.py --select=E,W,I 2>&1 | grep -A 5 "121\|122\|123\|124\|125\|126\|127"Repository: pathintegral-institute/mcpm.sh
Length of output: 160
Health endpoint will be protected by auth middleware; add typing and move imports to module level.
If auth_enabled is true (and stdio_mode is false), Docker/system health checks will fail because /health is protected by MCPMAuthMiddleware. Health checks typically cannot include API keys. Either exempt /health from authentication (if FastMCP supports route-level auth bypass) or document that health checks must include the Authorization header with the API key.
Additionally, move the JSONResponse import to module level and add type annotation to the request parameter:
+from starlette.requests import Request
+from starlette.responses import JSONResponse
+
# Add health check endpoint for Docker/System checks
@proxy.custom_route("/health", methods=["GET"])
- async def health_check(request):
- from starlette.responses import JSONResponse
-
+ async def health_check(_request: Request) -> JSONResponse:
return JSONResponse({"status": "healthy", "service": name or "mcpm-aggregated"})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mcpm/fastmcp_integration/proxy.py around lines 121 to 127, the /health
route is defined inline and will be protected by MCPMAuthMiddleware causing
automated Docker/system checks to fail; move the JSONResponse import to the
module top, add a type annotation for the request parameter (from
starlette.requests import Request -> async def health_check(request: Request):),
and either exempt the /health path from auth by updating the middleware to skip
requests.path == "/health" (or configure route-level bypass if supported) or
document that health checks must include the Authorization header with the API
key; implement the preferred fix (middleware path skip) so /health remains
unauthenticated.
1 participant
User description
This PR introduces automatic dependency isolation for Python-based MCP servers by leveraging
uv.Key Changes
piporpythonexecution, anduvis detected on the system, MCPM automatically upgrades the execution command touv run --with <package>.uv.Why this matters
This allows the MCPM Core to be upgraded to the latest dependencies (Pydantic v2, modern MCP SDK) without breaking compatibility with older servers that require legacy libraries.
PR Type
Enhancement, Bug fix
Description
Implement Auto-UVX injection for Python servers using
uv run --withfor dependency isolationConsolidate force operation logic with
should_force_operation()helper functionAdd explicit non-interactive mode support via
MCPM_NON_INTERACTIVEenvironment variableRedirect installation status message to stderr for cleaner output
Diagram Walkthrough
File Walkthrough
install.py
Auto-UVX injection and non-interactive mode supportsrc/mcpm/commands/install.py
shutilimport andshould_force_operationutility import forconsolidated force logic
prompt_with_default()to support explicit non-interactive modevia
MCPM_NON_INTERACTIVEenvironment variableinstance
forceflag checks withshould_force_operation(force)calls throughout
uvavailability andupgrades
python/pipcommands touv run --withfor dependencyisolation
uninstall.py
Consolidate force operation logicsrc/mcpm/commands/uninstall.py
should_force_operationutility functionforceflag check withshould_force_operation(force)call for confirmation logic
non_interactive.py
Add CLI force flag parameter to should_force_operationsrc/mcpm/utils/non_interactive.py
should_force_operation()signature to accept optionalcli_force_flagparameterMCPM_FORCEenvironment variable is setSummary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.