Second pass at implementing the mcp-agent CLI #462
Conversation
…improve live reload handling; add watchdog dependency for cli
…iguration for http/sse; add console print for http endpoint
…onfiguration not extracted properly; fix a console.print not showing in console
WalkthroughBumps CLI dependencies, wires multiple new Typer subcommands, adds a subprocess-based dev runner with optional watchdog live-reload, exposes HTTP endpoint reporting and immediate exit on serve shutdown, switches server config retrieval and explicit logging cleanup, tweaks build reporting, and adds a --servers option to invoke. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as dev command
participant Watcher as watchdog (optional)
participant Proc as Child Process
User->>CLI: mcp dev <script>
alt watchdog available
CLI->>Watcher: start watching (non-dir changes)
CLI->>Proc: spawn subprocess (sys.executable <script>)
loop monitor
CLI->>Proc: poll()
alt exited
Proc-->>CLI: exit code
CLI->>User: print "Process exited with code X"
end
Watcher-->>CLI: file change event
CLI->>Proc: terminate (wait up to 5s, kill if needed)
CLI->>Proc: restart subprocess
end
else no watchdog
CLI->>Proc: spawn subprocess
Proc-->>CLI: exit code (no reload)
end
User-->>CLI: Ctrl+C
CLI->>Proc: terminate (kill if needed)
CLI-->>User: stopping message
sequenceDiagram
autonumber
actor User
participant CLI as serve command
participant Uvicorn
participant App as mcp apps
User->>CLI: mcp serve --transport http|sse
alt transport=http
CLI->>App: select mcp.streamable_http_app
CLI->>Uvicorn: run host:port
CLI-->>User: print HTTP Endpoint http://host:port/mcp
else transport=sse
CLI->>App: select mcp.sse_app
CLI->>Uvicorn: run host:port
CLI-->>User: print SSE URL
end
Note over CLI,Uvicorn: On SIGINT/SIGTERM: schedule shutdown then os._exit(0)
sequenceDiagram
autonumber
actor User
participant CLI as invoke command
participant Agent as LLM/Agent Factory
User->>CLI: mcp invoke --agent ... --servers="a,b , c"
CLI->>CLI: parse servers -> ["a","b","c"]
CLI->>Agent: create with server_names=["a","b","c"]
Agent-->>CLI: responses
CLI-->>User: print messages/payloads with extra blank lines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
|
🔥 🔥 🔥 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyproject.toml (1)
110-121: Fix package discovery: hyphen vs underscore breaks setuptools find.include = ["mcp-agent"] won’t match the Python package (module names can’t contain '-') and will exclude your code at build time. Align both include and package-data keys to mcp_agent.
[tool.setuptools.packages.find] -include = ["mcp-agent"] +include = ["mcp_agent"] [tool.setuptools.package-data] -mcp-agent = [ # TODO: should this be mcp_agent? +mcp_agent = [src/mcp_agent/cli/commands/server.py (1)
541-543: Zip Slip risk when extracting .dxt archives.zipfile.extractall allows path traversal (e.g., ../../etc). Sanitize members before extracting.
- with zipfile.ZipFile(str(dxt_path), "r") as zf: - zf.extractall(base_extract_dir) + def _safe_members(zf): + for m in zf.infolist(): + p = _Path(m.filename) + if p.is_absolute() or ".." in p.parts: + continue + yield m + with zipfile.ZipFile(str(dxt_path), "r") as zf: + zf.extractall(base_extract_dir, members=list(_safe_members(zf)))src/mcp_agent/cli/main.py (1)
19-28: Guard cloud imports and alias registration to avoid import-time crashes when cloud extras are absent
from mcp_agent.cli.cloud.commands import ...is unguarded and will blow up on setups without cloud installed, even thoughcloud_appimport is wrapped. Also, alias commands are always registered. Make both conditional.Apply:
-# Mount existing cloud CLI -try: - from mcp_agent.cli.cloud.main import app as cloud_app # type: ignore -except Exception: # pragma: no cover - cloud is optional for non-cloud development - cloud_app = typer.Typer(help="Cloud commands (unavailable)") - -# Local command groups (scaffolded) -from mcp_agent.cli.cloud.commands import deploy_config, login, logout, whoami +# Mount existing cloud CLI +try: + from mcp_agent.cli.cloud.main import app as cloud_app # type: ignore + from mcp_agent.cli.cloud.commands import ( # type: ignore + deploy_config, + login, + logout, + whoami, + ) + _CLOUD_AVAILABLE = True +except Exception: # pragma: no cover - cloud is optional for non-cloud development + cloud_app = typer.Typer(help="Cloud commands (unavailable)") + deploy_config = login = logout = whoami = None # type: ignore + _CLOUD_AVAILABLE = False-# Mount cloud commands -app.add_typer(cloud_app, name="cloud", help="MCP Agent Cloud commands") - -# Register some key cloud commands directly as top-level commands -app.command("deploy", help="Deploy an MCP agent (alias for 'cloud deploy')")( - deploy_config -) -app.command( - "login", help="Authenticate to MCP Agent Cloud API (alias for 'cloud login')" -)(login) -app.command( - "whoami", help="Print current identity and org(s) (alias for 'cloud whoami')" -)(whoami) -app.command("logout", help="Clear credentials (alias for 'cloud logout')")(logout) +# Mount cloud commands +if _CLOUD_AVAILABLE: + app.add_typer(cloud_app, name="cloud", help="MCP Agent Cloud commands") + # Register key cloud commands as top-level aliases + app.command("deploy", help="Deploy an MCP agent (alias for 'cloud deploy')")(deploy_config) # type: ignore[arg-type] + app.command("login", help="Authenticate to MCP Agent Cloud API (alias for 'cloud login')")(login) # type: ignore[arg-type] + app.command("whoami", help="Print current identity and org(s) (alias for 'cloud whoami')")(whoami) # type: ignore[arg-type] + app.command("logout", help="Clear credentials (alias for 'cloud logout')")(logout) # type: ignore[arg-type] +else: + app.add_typer(cloud_app, name="cloud", help="Cloud commands (unavailable)")Also applies to: 150-164
🧹 Nitpick comments (13)
src/mcp_agent/cli/commands/build.py (2)
105-112: Tighten secrets file permission check (include group perms).Current check only flags world-readable (others). Secrets should also not be group-readable/writable. Consider treating any group/other rwx bits as insecure.
- if "secrets" in path.name: - stat_info = path.stat() - mode = stat_info.st_mode - # Check if others have read access - result["secure"] = not bool(mode & 0o004) - result["permissions"] = oct(mode)[-3:] + if "secrets" in path.name: + stat_info = path.stat() + mode = stat_info.st_mode + # Secure if no group/other permissions at all (owner-only) + result["secure"] = (mode & 0o077) == 0 + result["permissions"] = oct(mode & 0o777)[-3:]
351-360: Optional: add actionable hint when no servers.When no servers are configured, consider printing a one-liner to guide users, e.g., “mcp-agent server recipes” or “mcp-agent server discover”.
src/mcp_agent/cli/commands/invoke.py (3)
30-33: servers option parsing is fine; consider list-type option for UX.Current comma-split works. As a polish, Typer can accept multiple via List[str] (e.g., --server name1 --server name2) to avoid comma edge cases and allow shell completion per item.
-servers: Optional[str] = typer.Option( - None, "--servers", help="Comma-separated list of MCP server names" -), +servers: Optional[list[str]] = typer.Option( + None, "--server", help="Repeatable: MCP server name", rich_help_panel="MCP" +), @@ - server_list = servers.split(",") if servers else [] - server_list = [s.strip() for s in server_list if s.strip()] + server_list = [s.strip() for s in (servers or []) if s.strip()]Also applies to: 56-61
27-27: Avoid shadowing built-in 'vars'.Renaming to inputs_json or structured_vars avoids overshadowing vars().
- vars: Optional[str] = typer.Option(None, "--vars", help="JSON structured inputs"), + inputs_json: Optional[str] = typer.Option(None, "--vars", help="JSON structured inputs"), @@ - payload = json.loads(vars) if vars else {} + payload = json.loads(inputs_json) if inputs_json else {}
108-114: Minor import hygiene.Path is used in _run; importing it at the top improves readability and static analysis.
src/mcp_agent/cli/commands/serve.py (2)
309-316: Adjust printed scheme when TLS is enabled.If ssl_certfile/keyfile are set, print https:// in URLs.
- console.print(f"[bold]URL:[/bold] http://{host}:{port or 8000}") + scheme = "https" if (ssl_certfile and ssl_keyfile) else "http" + console.print(f"[bold]URL:[/bold] {scheme}://{host}:{port or 8000}") @@ - console.print(f"[bold]SSE:[/bold] http://{host}:{port or 8000}/sse") + console.print(f"[bold]SSE:[/bold] {scheme}://{host}:{port or 8000}/sse") @@ - f"[bold]HTTP:[/bold] http://{host}:{port or 8000}/mcp" + f"[bold]HTTP:[/bold] {scheme}://{host}:{port or 8000}/mcp"
290-299: Add a getattr fallback to mcp.app for streamable_http_app / sse_appFastMCP exposes streamable_http_app and sse_app (added with streamable‑http support), but keep a getattr fallback to mcp.app for compatibility.
- uvicorn_config = uvicorn.Config( - mcp.streamable_http_app if transport == "http" else mcp.sse_app, + app_attr = "streamable_http_app" if transport == "http" else "sse_app" + app_callable = getattr(mcp, app_attr, None) or getattr(mcp, "app", None) + uvicorn_config = uvicorn.Config( + app_callable,src/mcp_agent/cli/commands/server.py (1)
763-765: Same Rich 'end' kw check applies here.Ensure Console.print accepts end="\n\n" in your pinned Rich. Use explicit blank prints otherwise.
src/mcp_agent/cli/main.py (1)
60-66: Avoid double “no subcommand” messagingWith
no_args_is_help=True, Typer already shows help when no subcommand is given. The manual overview block becomes redundant and may duplicate output. Suggest removing it.- # If no subcommand given, show brief overview - if ctx.invoked_subcommand is None: - console.print("mcp-agent - Model Context Protocol agent CLI\n") - console.print("Run 'mcp-agent --help' to see all commands.")Also applies to: 120-124
src/mcp_agent/cli/commands/dev.py (4)
24-26: Honor global --no-color (and other globals) passed from mainThe dev command doesn’t accept
ctx, so global options (e.g.,--no-color) aren’t applied. Accepttyper.Contextand toggle the console accordingly.-@app.callback(invoke_without_command=True) -def dev(script: Path = typer.Option(Path("agent.py"), "--script")) -> None: +@app.callback(invoke_without_command=True) +def dev( + ctx: typer.Context, + script: Path = typer.Option(Path("agent.py"), "--script"), +) -> None: """Run the user's app script with optional live reload and preflight checks.""" + # Honor global color setting propagated by main + if getattr(ctx, "obj", None) and not ctx.obj.get("color", True): + console.no_color = True
28-39: More robust stdio command check (handle str or list command forms)Some configs use
command: "tool"while others usecommand: ["tool", "arg"].shutil.whichon a full string will fail. Parse the executable first.+import shlex @@ - for name, s in servers.items(): - if s.transport == "stdio" and s.command and not shutil.which(s.command): + for name, s in servers.items(): + if s.transport != "stdio" or not s.command: + continue + # s.command may be a string or a list + if isinstance(s.command, (list, tuple)): + prog = s.command[0] if s.command else "" + else: + try: + prog = shlex.split(s.command)[0] + except Exception: + prog = str(s.command).split()[0] if s.command else "" + if prog and not shutil.which(prog): console.print( - f"[yellow]Missing command for server '{name}': {s.command}[/yellow]" + f"[yellow]Missing command for server '{name}': {prog}[/yellow]" ) ok = FalseTo confirm schema variants in your repo, you can grep server configs for
command:values and types. Want a quick script to summarize them?
41-51: Fail fast if script missing and run from script directory for predictable relative pathsImprove UX by erroring early when the script isn’t found and by setting
cwdto the script’s folder so relative imports/paths behave as during normal execution.def _run_script() -> subprocess.Popen: """Run the script as a subprocess.""" - console.print(f"Running {script}") + console.print(f"Running {script}") + if not script.exists(): + console.print(f"[red]Script not found: {script}[/red]") + raise typer.Exit(2) # Run the script with the same Python interpreter return subprocess.Popen( - [sys.executable, str(script)], + [sys.executable, str(script)], + cwd=str(script.parent), stdout=None, # Inherit stdout stderr=None, # Inherit stderr stdin=None, # Inherit stdin )
97-104: Optional: terminate process trees to avoid orphaned grandchildren on restartIf the script spawns children (e.g., uvicorn),
terminate()may leave them running. Consider creating a process group and signaling the group.+import os +import signal @@ - process.terminate() + # Terminate the whole process group where possible + try: + if os.name != "nt": + os.killpg(os.getpgid(process.pid), signal.SIGTERM) + else: + process.terminate() + except Exception: + process.terminate() @@ - process = _run_script() + process = _run_script()And when spawning:
- return subprocess.Popen( + kwargs = {} + if os.name != "nt": + kwargs["preexec_fn"] = os.setsid # start new process group + else: + kwargs["creationflags"] = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + return subprocess.Popen( [sys.executable, str(script)], cwd=str(script.parent), stdout=None, stderr=None, stdin=None, + **kwargs, )If you prefer to keep it simpler, you can skip this; just flagging the edge case.
📜 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 (7)
pyproject.toml(2 hunks)src/mcp_agent/cli/commands/build.py(1 hunks)src/mcp_agent/cli/commands/dev.py(3 hunks)src/mcp_agent/cli/commands/invoke.py(5 hunks)src/mcp_agent/cli/commands/serve.py(4 hunks)src/mcp_agent/cli/commands/server.py(5 hunks)src/mcp_agent/cli/main.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/mcp_agent/cli/commands/invoke.py (1)
src/mcp_agent/workflows/factory.py (1)
create_llm(68-86)
src/mcp_agent/cli/commands/serve.py (1)
src/mcp_agent/core/context.py (1)
mcp(102-103)
src/mcp_agent/cli/main.py (9)
src/mcp_agent/cli/commands/chat.py (1)
chat(90-713)src/mcp_agent/cli/commands/dev.py (1)
dev(25-132)src/mcp_agent/cli/commands/invoke.py (1)
invoke(23-113)src/mcp_agent/cli/utils/typer_utils.py (1)
invoke(33-40)src/mcp_agent/cli/commands/serve.py (1)
serve(77-373)src/mcp_agent/cli/commands/build.py (1)
build(192-433)src/mcp_agent/cli/commands/logs.py (1)
logs(158-297)src/mcp_agent/cli/commands/doctor.py (1)
doctor(197-419)src/mcp_agent/cli/commands/configure.py (1)
configure(84-144)
src/mcp_agent/cli/commands/server.py (3)
src/mcp_agent/config.py (4)
Settings(583-690)MCPServerSettings(52-113)MCPSettings(116-124)get_settings(727-826)src/mcp_agent/cli/utils/importers.py (1)
import_servers_from_mcp_json(40-82)src/mcp_agent/core/context.py (1)
cleanup_context(251-264)
🔇 Additional comments (10)
pyproject.toml (1)
76-82: Confirm watchdog 6.x compatibility and platform wheels.PyPI shows watchdog 6.0.0 (released 2024-11-01) requires Python >=3.9 and lists support for 3.10–3.12; 6.x contains only minor API/behavior tweaks (inotify now uses select.poll, small CLI/utility removals). Verify binary wheels exist for your target OS/architectures (or allow source installs) and run the full test suite in your dev runner/CI to catch any environment-specific issues.
src/mcp_agent/cli/commands/build.py (1)
381-383: Nice UX: explicit message when no servers are configured.Good addition—prevents an empty table from confusing users.
src/mcp_agent/cli/commands/invoke.py (2)
19-19: Good call disabling color for piping.Console(color_system=None) improves log piping and JSON post-processing.
67-67: Resolved — Rich Console.print 'end' kwarg supported (Rich v13.9+) Verified: Console.print signature includes end: str = "\n", so console.print(res, end="\n\n\n") is valid; applies to lines 67, 77 and 106–107.src/mcp_agent/cli/commands/serve.py (1)
195-196: HTTP endpoint row is a helpful clarity boost.Good addition—distinguishing SSE and HTTP paths reduces confusion.
src/mcp_agent/cli/commands/server.py (4)
15-18: Good move to centralize settings loading and ensure cleanup.Using get_settings() and cleanup_context(shutdown_logger=True) aligns CLI flows and shuts down logging deterministically.
342-344: LGTM: list_servers now reads from resolved settings.This matches the new global settings flow.
447-451: LGTM: add() now operates on get_settings() and MCPSettings defaults.Consistent with the rest of the CLI.
779-780: Explicit logging shutdown after probe is a good addition.Prevents lingering OTEL/exporter threads in CLI workflows.
src/mcp_agent/cli/main.py (1)
136-148: New subcommands wiring looks goodMounting chat/dev/invoke/serve/server/build/logs/doctor/configure under the top-level app is consistent and aligns with the PR goals.
| def signal_handler(sig, frame): | ||
| console.print("\n[yellow]Shutting down server...[/yellow]") | ||
| shutdown_event.set() | ||
| os._exit(0) |
There was a problem hiding this comment.
Don’t call os._exit(0) in signal handler. It bypasses cleanup and can corrupt logs/metrics.
Immediate exit prevents graceful shutdown, resource cleanup, and OTEL/exporter flushes. Use a cooperative shutdown signal instead.
- def signal_handler(sig, frame):
+ # Keep a reference to the uvicorn server for cooperative shutdown
+ server_ref = {"srv": None}
+
+ def signal_handler(sig, frame):
console.print("\n[yellow]Shutting down server...[/yellow]")
shutdown_event.set()
- os._exit(0)
+ # For HTTP/SSE, ask uvicorn to exit if running
+ srv = server_ref.get("srv")
+ if srv is not None:
+ try:
+ srv.should_exit = True
+ except Exception:
+ pass
@@
- server = uvicorn.Server(uvicorn_config)
+ server = uvicorn.Server(uvicorn_config)
+ server_ref["srv"] = server📝 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.
| os._exit(0) | |
| # Keep a reference to the uvicorn server for cooperative shutdown | |
| server_ref = {"srv": None} | |
| def signal_handler(sig, frame): | |
| console.print("\n[yellow]Shutting down server...[/yellow]") | |
| shutdown_event.set() | |
| # For HTTP/SSE, ask uvicorn to exit if running | |
| srv = server_ref.get("srv") | |
| if srv is not None: | |
| try: | |
| srv.should_exit = True | |
| except Exception: | |
| pass | |
| # allow the process to exit naturally after cooperative shutdown |
| os._exit(0) | |
| server = uvicorn.Server(uvicorn_config) | |
| server_ref["srv"] = server |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/serve.py around line 256, replace the direct
os._exit(0) call in the signal handler with a cooperative shutdown mechanism:
set a shared shutdown flag or threading.Event (or call a provided shutdown
coroutine/function) so the main loop can detect the signal, perform graceful
teardown (close resources, flush logs/metrics/OTEL exporters) and then exit
normally; remove any use of os._exit in handlers and ensure the main thread
performs cleanup and returns an appropriate exit code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
33-33: Typer base bump aligned — mismatch resolved.This addresses the previous feedback about aligning Typer versions between base and CLI extras. Looks good.
🧹 Nitpick comments (1)
pyproject.toml (1)
80-81: Trim extras to avoid redundant deps; consider pinning upper bounds.
- You already depend on rich at the base level;
typer[all]pulls rich (and shellingham/colorama) again. Unless you needshellingham/colorama, prefer plaintyperto keep the CLI extra lean.- Since Typer is <1.0, consider adding an upper bound to avoid accidental breakage on future minor bumps. Same for watchdog major.
Option A (drop extras; leaner deps):
- "typer[all]>=0.15.3", + "typer>=0.15.3",Option B (keep shell completion, but be explicit):
- "typer[all]>=0.15.3", + "typer>=0.15.3", + "shellingham>=1.5.0",Optional safety pins:
- "typer>=0.15.3", + "typer>=0.15.3,<0.16", - "watchdog>=6.0.0" + "watchdog>=6.0.0,<7.0.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(2 hunks)src/mcp_agent/cli/main.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_agent/cli/main.py
Summary by CodeRabbit
New Features
Changes
Chores