Refactor CLI: dev umbrella, default main.py, smart server defaults, clearer init #469
Conversation
WalkthroughReworks CLI dispatch and top-level commands into a curated set and a new "dev" umbrella group; adds detect_default_script and select_servers_from_config utilities; centralizes script auto-detection and server resolution across many commands; makes init template script selectable and updates examples/messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (__main__/main)
participant Dev as dev_group
participant Cmd as Subcommand
User->>CLI: mcp-agent [args]
alt first arg ∈ KNOWN
CLI->>Cmd: Dispatch directly
else first arg == "go"
CLI->>Dev: Inject "dev"
Dev->>Cmd: go
else any arg matches GO_OPTIONS
CLI->>Dev: Inject "dev" + "go"
Dev->>Cmd: go
end
note right of Dev: dev_group callback → dev start when no subcommand
sequenceDiagram
autonumber
participant Cmd as Command (go/chat/serve/invoke/dev)
participant Utils1 as detect_default_script
participant Utils2 as select_servers_from_config
Cmd->>Utils1: detect_default_script(script)
Utils1-->>Cmd: resolved_script
Cmd->>Utils2: select_servers_from_config(csv, url_servers, stdio_servers)
Utils2-->>Cmd: resolved_server_list
Cmd->>Cmd: Execute using resolved_script and resolved_server_list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcp_agent/cli/commands/chat.py (1)
575-581: Use resolved_server_list for /prompt to honor smart defaultsMixing server_list here ignores config/dynamic defaults and can break tool/resource resolution.
- prompt_msgs = await ag.create_prompt( + prompt_msgs = await ag.create_prompt( prompt_name=prompt_name, arguments=arguments, - server_names=server_list or [], + server_names=resolved_server_list or [], )src/mcp_agent/cli/main.py (1)
189-197: Don't swallow typer.Exit; preserve expected exit codes (e.g., --version).Catching Exception here also catches typer.Exit, turning normal exits into failures. Handle typer.Exit separately and re-raise.
def run() -> None: """Run the CLI application.""" try: - app() - except Exception as e: + app() + except typer.Exit: + # Allow intended exits (e.g., --version) to propagate + raise + except Exception as e: # Unexpected errors - log full exception and show clean error to user logging.exception("Unhandled exception in CLI") print_error(f"An unexpected error occurred: {str(e)}") raise typer.Exit(1) from e
🧹 Nitpick comments (10)
src/mcp_agent/cli/commands/init.py (1)
103-103: Keep Python 3.9 compatibility: avoidstr | None.
|unions require Python 3.10+. If you haven’t raised the floor, useOptional[str].Apply:
- entry_script_name: str | None = None + entry_script_name: Optional[str] = NoneAnd add the import:
from __future__ import annotations +from typing import Optionalsrc/mcp_agent/cli/commands/doctor.py (1)
410-416: Update quick-start to also show 'serve' and 'uv run' pathsGreat swap to dev start. Consider listing HTTP serve and uv run to mirror PR guidance.
"• Start chat: [cyan]mcp-agent chat --model anthropic.haiku[/cyan]\n" - "• Run agent: [cyan]mcp-agent dev start --script main.py[/cyan]", + "• Run agent: [cyan]mcp-agent dev start --script main.py[/cyan]\n" + "• Serve over HTTP: [cyan]mcp-agent dev serve --script main.py --transport http --port 8000[/cyan]\n" + "• Run directly: [cyan]uv run main.py[/cyan]",src/mcp_agent/cli/commands/serve.py (1)
121-124: Docstring example still references agent.pyPrefer main.py to keep examples consistent.
- mcp-agent serve --script agent.py + mcp-agent serve --script main.pysrc/mcp_agent/cli/commands/dev.py (1)
53-55: Add explicit missing-script check for friendlier DXCurrently Python errors out if main.py/agent.py is absent. Mirror serve.py’s helpful message.
- # Resolve script path with auto-detection (main.py preferred) - script = detect_default_script(script) + # Resolve script path with auto-detection (main.py preferred) + script = detect_default_script(script) + if not script.exists(): + console.print(f"[red]Script not found: {script}[/red]") + console.print("\n[dim]Create a main.py (preferred) or agent.py file, or specify --script[/dim]") + raise typer.Exit(1)src/mcp_agent/cli/commands/invoke.py (2)
56-63: Handle missing script early to avoid raw tracebacksAlign with serve/test for consistent UX.
- script_path = detect_default_script(Path(script) if script else None) - app_obj = load_user_app(script_path) + script_path = detect_default_script(Path(script) if script else None) + if not script_path.exists(): + typer.secho(f"Script not found: {script_path}", err=True, fg=typer.colors.RED) + typer.secho("Create main.py (preferred) or agent.py, or pass --script", err=True, fg=typer.colors.YELLOW) + raise typer.Exit(1) + app_obj = load_user_app(script_path)
32-33: Avoid shadowing built-in 'vars'Rename local to vars_json (CLI flag remains --vars).
- vars: Optional[str] = typer.Option(None, "--vars", help="JSON structured inputs"), + vars_json: Optional[str] = typer.Option(None, "--vars", help="JSON structured inputs"), @@ - payload = json.loads(vars) if vars else {} + payload = json.loads(vars_json) if vars_json else {}Also applies to: 50-53
src/mcp_agent/cli/core/utils.py (1)
121-133: Deduplicate server names and preserve orderMerging inputs can introduce duplicates. Return stable-ordered unique names.
- if names: - return names + if names: + # Stable dedupe + return list(dict.fromkeys(names)) @@ - if settings.mcp and settings.mcp.servers: - return list(settings.mcp.servers.keys()) + if settings.mcp and settings.mcp.servers: + return list(settings.mcp.servers.keys())src/mcp_agent/cli/commands/go.py (2)
275-277: Add missing-script guard after auto-detectPrevents unhandled FileNotFoundError from load_user_app and matches serve/chat UX.
- # Resolve script with auto-detection - script = detect_default_script(script) + # Resolve script with auto-detection + script = detect_default_script(script) + if not script.exists(): + typer.secho(f"Script not found: {script}", err=True, fg=typer.colors.RED) + typer.secho("Create main.py (preferred) or agent.py, or pass --script", err=True, fg=typer.colors.YELLOW) + raise typer.Exit(1)
311-314: Optional: ensure unique server listIf both URL and stdio add same name, duplicates can leak through.
resolved_server_list = select_servers_from_config( ",".join(server_list) if server_list else None, url_servers, stdio_servers ) +# Stable dedupe +resolved_server_list = list(dict.fromkeys(resolved_server_list))src/mcp_agent/cli/commands/chat.py (1)
112-114: Add missing-script check after auto-detectMirror serve/test to avoid raw errors when no script is present.
# Resolve script with auto-detection script = detect_default_script(script) +if not script.exists(): + typer.secho(f"Script not found: {script}", err=True, fg=typer.colors.RED) + typer.secho("Create main.py (preferred) or agent.py, or pass --script", err=True, fg=typer.colors.YELLOW) + raise typer.Exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/mcp_agent/cli/__main__.py(1 hunks)src/mcp_agent/cli/commands/chat.py(15 hunks)src/mcp_agent/cli/commands/dev.py(2 hunks)src/mcp_agent/cli/commands/doctor.py(1 hunks)src/mcp_agent/cli/commands/go.py(5 hunks)src/mcp_agent/cli/commands/init.py(3 hunks)src/mcp_agent/cli/commands/invoke.py(2 hunks)src/mcp_agent/cli/commands/serve.py(3 hunks)src/mcp_agent/cli/core/utils.py(2 hunks)src/mcp_agent/cli/main.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/mcp_agent/cli/commands/dev.py (1)
src/mcp_agent/cli/core/utils.py (1)
detect_default_script(84-103)
src/mcp_agent/cli/commands/init.py (1)
src/mcp_agent/cli/commands/config.py (1)
_load_template(35-48)
src/mcp_agent/cli/core/utils.py (1)
src/mcp_agent/config.py (3)
MCPServerSettings(52-113)MCPSettings(116-124)get_settings(727-826)
src/mcp_agent/cli/commands/invoke.py (1)
src/mcp_agent/cli/core/utils.py (3)
load_user_app(30-72)detect_default_script(84-103)select_servers_from_config(106-132)
src/mcp_agent/cli/commands/serve.py (1)
src/mcp_agent/cli/core/utils.py (1)
detect_default_script(84-103)
src/mcp_agent/cli/__main__.py (1)
src/mcp_agent/cli/main.py (1)
main(104-143)
src/mcp_agent/cli/commands/go.py (1)
src/mcp_agent/cli/core/utils.py (2)
detect_default_script(84-103)select_servers_from_config(106-132)
src/mcp_agent/cli/main.py (3)
src/mcp_agent/cli/cloud/commands/auth/login/main.py (1)
login(57-103)src/mcp_agent/cli/utils/typer_utils.py (1)
HelpfulTyperGroup(13-40)src/mcp_agent/cli/commands/dev.py (1)
dev(26-136)
🔇 Additional comments (13)
src/mcp_agent/cli/commands/init.py (3)
227-228: LGTM: updated example usesuv run.
231-231: LGTM: factory example switched touv run.
208-221: Add Python fallback to printed run instructionsAdd a python fallback for users who may not have the 'uv' CLI.
File: src/mcp_agent/cli/commands/init.py (lines 208-221)
- console.print(f"3. Run your agent: [cyan]uv run {run_file}[/cyan]") + console.print(f"3. Run your agent: [cyan]uv run {run_file}[/cyan]") + console.print(f" (fallback) [cyan]python {run_file}[/cyan]")src/mcp_agent/cli/commands/serve.py (2)
141-149: Good: default script auto-detection with clear guidanceUsing detect_default_script and the improved error hint aligns UX across commands. LGTM.
384-391: Test command parity with serve: niceAuto-detect + friendly missing-script message is consistent. LGTM.
src/mcp_agent/cli/commands/dev.py (1)
26-26: CLI default change to None is correctShifts responsibility to detect_default_script. Good.
src/mcp_agent/cli/core/utils.py (1)
84-104: Solid default script selectionOrder and fallback are sensible and used consistently across commands. LGTM.
src/mcp_agent/cli/__main__.py (1)
46-57: Back-compat routing is clear and minimalNice injection strategy for go and chat-like options without impacting curated commands.
src/mcp_agent/cli/commands/chat.py (1)
148-152: Centralized server resolution is correctResolved list respects explicit > dynamic > config precedence. LGTM.
src/mcp_agent/cli/main.py (4)
146-151: Top-level curated surface looks good; doctor addition is clear.The curated top-level keeps init/quickstart/config and adds doctor. Good separation between curated and dev/runtime.
152-173: Dev umbrella structure is coherent.Subcommand set under dev (start, chat, invoke, serve, server, build, logs, check, go, keys, models, client) aligns with the PR’s goals. Nice consolidation.
174-175: Mounting the dev umbrella at top-level is appropriate.This gives a clear entry for local workflows without polluting the top-level.
28-28: Exports present — verify Typer-compatible signatures for deploy_config and login.
- Exports found: src/mcp_agent/cli/cloud/commands/init.py (includes deploy_config, login) and src/mcp_agent/cli/cloud/commands/deploy/init.py (all = ["deploy_config"]).
- Definitions: src/mcp_agent/cli/cloud/commands/deploy/main.py -> def deploy_config(...); src/mcp_agent/cli/cloud/commands/auth/login/main.py -> def login(...).
- Action: Confirm each function’s signature is Typer-compatible (typed parameters and defaults or use of typer.Option/Argument) so they can be used as CLI commands at runtime.
| # Determine entry script name and handle existing files | ||
| script_name = "main.py" | ||
| script_path = dir / script_name | ||
| agent_content = _load_template("basic_agent.py") | ||
| if agent_content and _write(agent_path, agent_content, force): | ||
| files_created.append("agent.py") | ||
| # Make executable | ||
| try: | ||
| agent_path.chmod(agent_path.stat().st_mode | 0o111) | ||
| except Exception: | ||
| pass | ||
|
|
||
| if agent_content: | ||
| write_force_flag = force | ||
| if script_path.exists() and not force: | ||
| if Confirm.ask(f"{script_path} exists. Overwrite?", default=False): | ||
| write_force_flag = True | ||
| else: | ||
| # Ask for an alternate filename and ensure it ends with .py | ||
| alt_name = Prompt.ask( | ||
| "Enter a filename to save the agent", default="agent.py" | ||
| ) | ||
| if not alt_name.endswith(".py"): | ||
| alt_name += ".py" | ||
| script_name = alt_name | ||
| script_path = dir / script_name | ||
| # keep write_force_flag as-is to allow overwrite prompt if needed | ||
|
|
||
| if _write(script_path, agent_content, write_force_flag): | ||
| files_created.append(script_name) | ||
| entry_script_name = script_name | ||
| # Make executable | ||
| try: | ||
| script_path.chmod(script_path.stat().st_mode | 0o111) | ||
| except Exception: | ||
| pass | ||
|
|
||
| elif template == "server": |
There was a problem hiding this comment.
Harden alt filename handling to prevent path escapes.
A user can enter an absolute path (e.g., “/tmp/x.py”) or include “../”, which would write outside the target dir. Constrain to a basename and ensure the resolved path remains within dir.
- alt_name = Prompt.ask(
- "Enter a filename to save the agent", default="agent.py"
- )
- if not alt_name.endswith(".py"):
- alt_name += ".py"
- script_name = alt_name
- script_path = dir / script_name
+ alt_name = Prompt.ask(
+ "Enter a filename to save the agent", default="agent.py"
+ )
+ if not alt_name.endswith(".py"):
+ alt_name += ".py"
+ base_dir = dir.resolve()
+ # Keep only basename and ensure path stays within target dir
+ script_name = Path(alt_name).name
+ candidate_path = (base_dir / script_name).resolve()
+ try:
+ candidate_path.relative_to(base_dir)
+ except ValueError:
+ console.print("[red]Filename must be within the target directory[/red]")
+ raise typer.Exit(1)
+ script_path = candidate_path
+ if script_path.exists() and script_path.is_dir():
+ console.print(f"[red]{script_path} is a directory; choose a .py filename[/red]")
+ raise typer.Exit(1)📝 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.
| # Determine entry script name and handle existing files | |
| script_name = "main.py" | |
| script_path = dir / script_name | |
| agent_content = _load_template("basic_agent.py") | |
| if agent_content and _write(agent_path, agent_content, force): | |
| files_created.append("agent.py") | |
| # Make executable | |
| try: | |
| agent_path.chmod(agent_path.stat().st_mode | 0o111) | |
| except Exception: | |
| pass | |
| if agent_content: | |
| write_force_flag = force | |
| if script_path.exists() and not force: | |
| if Confirm.ask(f"{script_path} exists. Overwrite?", default=False): | |
| write_force_flag = True | |
| else: | |
| # Ask for an alternate filename and ensure it ends with .py | |
| alt_name = Prompt.ask( | |
| "Enter a filename to save the agent", default="agent.py" | |
| ) | |
| if not alt_name.endswith(".py"): | |
| alt_name += ".py" | |
| script_name = alt_name | |
| script_path = dir / script_name | |
| # keep write_force_flag as-is to allow overwrite prompt if needed | |
| if _write(script_path, agent_content, write_force_flag): | |
| files_created.append(script_name) | |
| entry_script_name = script_name | |
| # Make executable | |
| try: | |
| script_path.chmod(script_path.stat().st_mode | 0o111) | |
| except Exception: | |
| pass | |
| elif template == "server": | |
| # Determine entry script name and handle existing files | |
| script_name = "main.py" | |
| script_path = dir / script_name | |
| agent_content = _load_template("basic_agent.py") | |
| if agent_content: | |
| write_force_flag = force | |
| if script_path.exists() and not force: | |
| if Confirm.ask(f"{script_path} exists. Overwrite?", default=False): | |
| write_force_flag = True | |
| else: | |
| # Ask for an alternate filename and ensure it ends with .py | |
| alt_name = Prompt.ask( | |
| "Enter a filename to save the agent", default="agent.py" | |
| ) | |
| if not alt_name.endswith(".py"): | |
| alt_name += ".py" | |
| base_dir = dir.resolve() | |
| # Keep only basename and ensure path stays within target dir | |
| script_name = Path(alt_name).name | |
| candidate_path = (base_dir / script_name).resolve() | |
| try: | |
| candidate_path.relative_to(base_dir) | |
| except ValueError: | |
| console.print("[red]Filename must be within the target directory[/red]") | |
| raise typer.Exit(1) | |
| script_path = candidate_path | |
| if script_path.exists() and script_path.is_dir(): | |
| console.print(f"[red]{script_path} is a directory; choose a .py filename[/red]") | |
| raise typer.Exit(1) | |
| # keep write_force_flag as-is to allow overwrite prompt if needed | |
| if _write(script_path, agent_content, write_force_flag): | |
| files_created.append(script_name) | |
| entry_script_name = script_name | |
| # Make executable | |
| try: | |
| script_path.chmod(script_path.stat().st_mode | 0o111) | |
| except Exception: | |
| pass | |
| elif template == "server": |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/init.py around lines 126-156, the alternate
filename from Prompt.ask may contain absolute paths or path traversal (e.g.,
“/tmp/x.py” or “../escape.py”); sanitize this by taking only the basename (e.g.,
Path(alt_name).name), ensure it ends with “.py”, recompute script_path as dir /
sanitized_name, then verify the resolved script_path remains inside dir (e.g.,
compare dir.resolve() as a parent of script_path.resolve(), or fall back to a
safe default filename if the check fails); keep the existing write_force_flag
behavior and subsequent permission/chmod steps.
| console.print("3. Run the server: [cyan]uv run server.py[/cyan]") | ||
| console.print( | ||
| " Or serve: [cyan]mcp-agent serve --script server.py[/cyan]" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Command mismatch with CLI reorg: use mcp-agent dev serve.
Runtime commands moved under dev. This line still suggests the old top-level serve.
- console.print(
- " Or serve: [cyan]mcp-agent serve --script server.py[/cyan]"
- )
+ console.print(
+ " Or serve: [cyan]mcp-agent dev serve --script server.py[/cyan]"
+ )Search for other stale references:
🏁 Script executed:
#!/bin/bash
rg -nP -C1 '\bmcp-agent\s+serve\b'Length of output: 1811
Replace remaining mcp-agent serve usages with mcp-agent dev serve
Runtime commands were moved under the dev group — update CLI help/messages and docs.
- src/mcp_agent/cli/commands/init.py (lines ~223-225): change the printed hint to
mcp-agent dev serve.
- console.print(
- " Or serve: [cyan]mcp-agent serve --script server.py[/cyan]"
- )
+ console.print(
+ " Or serve: [cyan]mcp-agent dev serve --script server.py[/cyan]"
+ )- src/mcp_agent/cli/commands/serve.py (examples & output: ~lines 121-123, 471-472, 533-539): update all example and status strings to use
mcp-agent dev serve(andmcp-agent dev serve testwhere applicable). - docs/cloud/agent-server.mdx (lines ~140, 145): update
uv run mcp-agent serve --app ...touv run mcp-agent dev serve --app ....
📝 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.
| console.print("3. Run the server: [cyan]uv run server.py[/cyan]") | |
| console.print( | |
| " Or serve: [cyan]mcp-agent serve --script server.py[/cyan]" | |
| ) | |
| console.print("3. Run the server: [cyan]uv run server.py[/cyan]") | |
| console.print( | |
| " Or serve: [cyan]mcp-agent dev serve --script server.py[/cyan]" | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/init.py around lines 222 to 225, update the
printed CLI hint that currently suggests "mcp-agent serve" to the new runtime
command "mcp-agent dev serve"; replace both occurrences so the messages read "3.
Run the server: uv run server.py" and "Or serve: mcp-agent dev serve --script
server.py" (i.e., change the `mcp-agent serve` text to `mcp-agent dev serve`).
| @dev_group.callback(invoke_without_command=True) | ||
| def _dev_group_entry( | ||
| ctx: typer.Context, | ||
| script: Path = typer.Option(None, "--script", help="Entry script"), | ||
| ): | ||
| """If no subcommand is provided, behave like 'dev start'.""" | ||
| if ctx.invoked_subcommand: | ||
| return | ||
| # Delegate to the existing dev implementation | ||
| dev_cmd.dev(script=script) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make --script optional and avoid it being silently ignored with subcommands.
Two fixes:
- Type correctness: default None with type Path should be Optional[Path].
- UX: mcp-agent dev --script app.py start currently ignores the group-level --script. Warn users when a subcommand is present so intent isn’t lost.
@dev_group.callback(invoke_without_command=True)
def _dev_group_entry(
ctx: typer.Context,
- script: Path = typer.Option(None, "--script", help="Entry script"),
+ script: Optional[Path] = typer.Option(None, "--script", help="Entry script"),
):
"""If no subcommand is provided, behave like 'dev start'."""
- if ctx.invoked_subcommand:
- return
+ if ctx.invoked_subcommand:
+ if script is not None:
+ # Inform user that group-level --script is ignored for subcommands
+ err_console.print(
+ "[yellow]Note: --script passed to 'dev' is ignored when using a subcommand. "
+ "Pass --script after the subcommand instead (e.g., 'mcp-agent dev start --script app.py').[/yellow]"
+ )
+ return
# Delegate to the existing dev implementation
dev_cmd.dev(script=script)Also add the missing import (outside this hunk):
from typing import OptionalVerification:
- mcp-agent dev --script app.py → runs dev start with app.py.
- mcp-agent dev --script app.py start → shows the warning; start proceeds with its own option parsing.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Search for @dev_group.callback occurrences"
rg -n --hidden -S "@dev_group.callback" || true
echo
echo "2) Print src/mcp_agent/cli/main.py (first 240 lines) if it exists"
if [ -f "src/mcp_agent/cli/main.py" ]; then
sed -n '1,240p' src/mcp_agent/cli/main.py || true
else
echo "src/mcp_agent/cli/main.py not found"
fi
echo
echo "3) Search for def _dev_group_entry"
rg -n --hidden -S "def _dev_group_entry" || true
echo
echo "4) Search for dev_cmd references"
rg -n --hidden -S "dev_cmd" || true
echo
echo "5) Search for a 'dev' function definition"
rg -n --hidden -S "def dev\(" || true
echo
echo "6) Look for dev subcommands (decorators or defs named start)"
rg -n --hidden -S "@dev_group.command|def start\(" || true
echo
echo "7) Search for err_console / console.print / typer.secho usage"
rg -n --hidden -S "err_console|console\.print|typer\.secho|typer\.echo" || true
echo
echo "8) Search for typing imports or Optional usage"
rg -n --hidden -S "from typing import|Optional" || trueLength of output: 206664
Make --script optional and warn when a subcommand is present.
Verified: dev_cmd.dev also declares its own --script (src/mcp_agent/cli/commands/dev.py:26), so a group-level --script passed before a subcommand will be silently ignored — fix both typing and UX.
File: src/mcp_agent/cli/main.py (callback around lines 77–87)
@dev_group.callback(invoke_without_command=True)
def _dev_group_entry(
ctx: typer.Context,
- script: Path = typer.Option(None, "--script", help="Entry script"),
+ script: Optional[Path] = typer.Option(None, "--script", help="Entry script"),
):
"""If no subcommand is provided, behave like 'dev start'."""
- if ctx.invoked_subcommand:
- return
+ if ctx.invoked_subcommand:
+ if script is not None:
+ # Inform user that group-level --script is ignored for subcommands
+ err_console.print(
+ "[yellow]Note: --script passed to 'dev' is ignored when using a subcommand. "
+ "Pass --script after the subcommand instead (e.g., 'mcp-agent dev start --script app.py').[/yellow]"
+ )
+ return
# Delegate to the existing dev implementation
dev_cmd.dev(script=script)Also add (near other typing imports):
from typing import Optional🤖 Prompt for AI Agents
In src/mcp_agent/cli/main.py around lines 77–87, the group callback's script
parameter should be Optional[Path] and you must warn if a group-level --script
is provided but a subcommand is invoked (since subcommands have their own
--script). Add "from typing import Optional" near the other typing imports,
change the callback signature to use Optional[Path] for script, and inside the
ctx.invoked_subcommand branch, if script is not None emit a warning (e.g.,
typer.secho or typer.echo with a clear message) that the group-level --script
will be ignored, then return without delegating to dev_cmd.dev; keep the current
behavior of delegating to dev_cmd.dev only when no subcommand is invoked.
…r instead of showing help menu
…re not overwritten
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/mcp_agent/cli/main.py (1)
77-86: Make script Optional and warn when group-level --script is ignored with subcommands.Prevents silent confusion when users pass --script before a subcommand (previously flagged in review).
-@dev_group.callback(invoke_without_command=True) -def _dev_group_entry( - ctx: typer.Context, - script: Path = typer.Option(None, "--script", help="Entry script"), -): +from typing import Optional + +@dev_group.callback(invoke_without_command=True) +def _dev_group_entry( + ctx: typer.Context, + script: Optional[Path] = typer.Option(None, "--script", help="Entry script"), +): """If no subcommand is provided, behave like 'dev start'.""" - if ctx.invoked_subcommand: - return + if ctx.invoked_subcommand: + if script is not None: + err_console.print( + "[yellow]Note: --script passed to 'dev' is ignored when using a subcommand. " + "Pass --script after the subcommand instead (e.g., 'mcp-agent dev start --script app.py').[/yellow]" + ) + return # Delegate to the existing dev implementation dev_cmd.dev(script=script)
🧹 Nitpick comments (5)
src/mcp_agent/cli/commands/keys.py (1)
47-49: Good endpoint switch; also handle rate limiting and tighten result messaging./v1/models is the right lightweight probe. Recommend treating HTTP 429 as “inconclusive/rate limited” instead of “Unexpected response”, so users aren’t told their key is invalid during transient throttling.
- if response.status_code in [200, 401, 403]: + if response.status_code in [200, 401, 403, 429]: if response.status_code == 200: return True, "Key is valid" - else: + elif response.status_code in (401, 403): return False, f"Invalid key (HTTP {response.status_code})" + else: # 429 + return False, "Rate limited (HTTP 429) — try again shortly" else: return False, f"Unexpected response (HTTP {response.status_code})"src/mcp_agent/cli/commands/serve.py (1)
141-149: Nice: auto-detects main.py (preferred) with a clear missing-script hint.Minor UX polish: when TLS is configured, print https:// in the Info panel and startup logs.
- address = f"{host}:{port or 8000}" - info_table.add_row("Address", f"http://{address}") + address = f"{host}:{port or 8000}" + scheme = "https" if (ssl_certfile and ssl_keyfile) else "http" + info_table.add_row("Address", f"{scheme}://{address}") if transport == "sse": - info_table.add_row("SSE Endpoint", f"http://{address}/sse") + info_table.add_row("SSE Endpoint", f"{scheme}://{address}/sse") elif transport == "http": - info_table.add_row("HTTP Endpoint", f"http://{address}/mcp") + info_table.add_row("HTTP Endpoint", f"{scheme}://{address}/mcp")- 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}") if transport == "sse": - console.print(f"[bold]SSE:[/bold] http://{host}:{port or 8000}/sse") + console.print(f"[bold]SSE:[/bold] {scheme}://{host}:{port or 8000}/sse") elif transport == "http": - console.print(f"[bold]HTTP:[/bold] http://{host}:{port or 8000}/mcp") + console.print(f"[bold]HTTP:[/bold] {scheme}://{host}:{port or 8000}/mcp")src/mcp_agent/cli/commands/chat.py (2)
113-115: Early script auto-detection is solid; consider friendlier error if missing.Mirror serve.py’s message when neither main.py nor agent.py exists to reduce confusion before load_user_app raises.
script = detect_default_script(script) +if not script.exists(): + typer.secho( + f"Script not found: {script}\nCreate a main.py (preferred) or agent.py, or pass --script.", + err=True, + fg=typer.colors.RED, + ) + raise typer.Exit(1)
149-153: De-duplicate server names from multiple sources.select_servers_from_config may return duplicates when URL and stdio names overlap. Prefer stable-order de-dupe in the utility.
Outside this file (src/mcp_agent/cli/core/utils.py):
def select_servers_from_config( @@ - if url_servers: - names.extend(list(url_servers.keys())) - if stdio_servers: - names.extend(list(stdio_servers.keys())) - if names: - return names + if url_servers: + names.extend(list(url_servers.keys())) + if stdio_servers: + names.extend(list(stdio_servers.keys())) + if names: + # stable-order de-dupe + names = list(dict.fromkeys(names)) + return namessrc/mcp_agent/cli/main.py (1)
69-74: Minor help text nit: s/servers/server.Subcommand mounted is “server”, not “servers”.
-dev_group = typer.Typer( - help="Local development: start app, chat, invoke, serve, servers, build, logs", +dev_group = typer.Typer( + help="Local development: start app, chat, invoke, serve, server, build, logs", no_args_is_help=False, cls=HelpfulTyperGroup, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mcp_agent/cli/commands/chat.py(15 hunks)src/mcp_agent/cli/commands/init.py(3 hunks)src/mcp_agent/cli/commands/keys.py(1 hunks)src/mcp_agent/cli/commands/serve.py(6 hunks)src/mcp_agent/cli/core/utils.py(5 hunks)src/mcp_agent/cli/main.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mcp_agent/cli/commands/init.py
- src/mcp_agent/cli/core/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcp_agent/cli/commands/chat.py (2)
src/mcp_agent/cli/core/utils.py (3)
detect_default_script(94-113)select_servers_from_config(116-142)load_user_app(30-82)src/mcp_agent/config.py (1)
get_settings(727-826)
src/mcp_agent/cli/commands/serve.py (1)
src/mcp_agent/cli/core/utils.py (2)
load_user_app(30-82)detect_default_script(94-113)
src/mcp_agent/cli/main.py (2)
src/mcp_agent/cli/utils/typer_utils.py (1)
HelpfulTyperGroup(13-40)src/mcp_agent/cli/commands/dev.py (1)
dev(26-136)
🔇 Additional comments (12)
src/mcp_agent/cli/commands/serve.py (4)
22-23: Import of detect_default_script is appropriate and aligns with new defaults.
121-124: Docs/examples updated tomcp-agent dev serve— looks good.
384-391: Test path uses detect_default_script consistently — good.
534-541: “Next steps” messaging aligns with new dev namespace.src/mcp_agent/cli/commands/chat.py (7)
15-21: Centralized imports (detect_default_script, select_servers_from_config, get_settings) — good cohesion.Also applies to: 26-26
105-106: Optional script + Path typing is correct for auto-detection use.
159-164: Disabling progress + passing settings_override yields cleaner output.
248-253: Using resolved_server_list for fan-out is correct.
299-301: Interactive /tools and /resources respect resolved servers — good.Also applies to: 319-321
393-395: One-shot paths correctly pass resolved servers.Also applies to: 731-733
419-424: REPL paths: consistent resolved server usage and cleaner UX.Also applies to: 445-449, 503-509, 531-535, 552-556
src/mcp_agent/cli/main.py (1)
146-176: CLI rewire under dev + top-level doctor/config/init/quickstart — solid.
| # Smart defaults for servers | ||
| resolved_server_list = select_servers_from_config( | ||
| servers_csv, url_servers, stdio_servers | ||
| ) | ||
|
|
There was a problem hiding this comment.
Smart server defaults wired — good. One functional inconsistency remains.
/prompt still uses server_list instead of resolved_server_list, so dynamic/configured servers are ignored there.
- prompt_msgs = await ag.create_prompt(
+ prompt_msgs = await ag.create_prompt(
prompt_name=prompt_name,
arguments=arguments,
- server_names=server_list or [],
+ server_names=resolved_server_list or [],
)📝 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.
| # Smart defaults for servers | |
| resolved_server_list = select_servers_from_config( | |
| servers_csv, url_servers, stdio_servers | |
| ) | |
| # Smart defaults for servers | |
| resolved_server_list = select_servers_from_config( | |
| servers_csv, url_servers, stdio_servers | |
| ) | |
| prompt_msgs = await ag.create_prompt( | |
| prompt_name=prompt_name, | |
| arguments=arguments, | |
| server_names=resolved_server_list or [], | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/chat.py around lines 149-153, the code computes
resolved_server_list but the /prompt handler still references server_list,
causing dynamic/configured servers to be ignored; update the /prompt command
handler to use resolved_server_list everywhere server_list is currently used
(pass resolved_server_list into any function calls, closures or command
construction), and add a safe fallback to resolved_server_list if server_list
may be None so the dynamic/configured server selection is honored.
💡 Verification agent
🧩 Analysis chain
Quick sweep for any other legacy server_list usages.
Run this to ensure no other call paths still rely on server_list for server_names.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=py -C2 '\bserver_names\s*=\s*server_list\b' || true
rg -nP --type=py -C2 '\bserver_names\s*=\s*server_list\s*\|\|\s*\[\]' || true
rg -nP --type=py -C2 'create_prompt\([^)]*server_names\s*=' || trueLength of output: 1248
Replace remaining legacy server_list usages with resolved_server_list.
rg found three call sites still passing server_list; update them to pass the resolved list (or call select_servers_from_config in that scope) and preserve or [] defaults where appropriate.
- src/mcp_agent/cli/commands/invoke.py:65 — server_names=server_list
- src/mcp_agent/cli/commands/go.py:113 — server_names=server_list or []
- src/mcp_agent/cli/commands/chat.py:591 — server_names=server_list or []
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/chat.py around lines 149-153, you've already
assigned resolved_server_list from select_servers_from_config; replace the
remaining legacy server_list usages in the codebase to use that resolved list:
update src/mcp_agent/cli/commands/invoke.py at ~line 65 to pass
server_names=resolved_server_list (or call select_servers_from_config there to
obtain it), update src/mcp_agent/cli/commands/go.py at ~line 113 to pass
server_names=resolved_server_list or [], and update
src/mcp_agent/cli/commands/chat.py at ~line 591 to pass
server_names=resolved_server_list or []; preserve the existing "or []" defaults
where present.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mcp_agent/cli/commands/chat.py (3)
105-105: Add a short alias for --script for ergonomics.Apply:
- script: Optional[Path] = typer.Option(None, "--script"), + script: Optional[Path] = typer.Option(None, "--script", "-s"),
113-115: Fail fast with helpful guidance when no script is found.Currently a missing main.py/agent.py raises FileNotFoundError from load_user_app later; provide a clearer CLI message here.
Apply:
# Resolve script with auto-detection script = detect_default_script(script) + + # Fail fast with clear guidance + if not script.exists(): + typer.secho( + f"Script not found: {script}. Run `mcp-agent init` to create main.py, " + "or pass --script <file>.", + err=True, + fg=typer.colors.RED, + ) + raise typer.Exit(2)
116-147: Remove vestigial server_list accumulation.server_list is built/extended here but no longer used after introducing select_servers_from_config; drop it to avoid confusion.
Apply:
- server_list = servers_csv.split(",") if servers_csv else None @@ - url_servers = generate_server_configs(parsed) - if url_servers and not server_list: - server_list = list(url_servers.keys()) - elif url_servers and server_list: - server_list.extend(list(url_servers.keys())) + url_servers = generate_server_configs(parsed) @@ - if stdio_servers: - if not server_list: - server_list = list(stdio_servers.keys()) - else: - server_list.extend(list(stdio_servers.keys())) + # stdio_servers resolved; selection happens via select_servers_from_config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp_agent/cli/commands/chat.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/mcp_agent/cli/commands/chat.py (2)
src/mcp_agent/cli/core/utils.py (3)
detect_default_script(94-113)select_servers_from_config(116-142)load_user_app(30-82)src/mcp_agent/config.py (1)
get_settings(727-826)
🔇 Additional comments (16)
src/mcp_agent/cli/commands/chat.py (16)
19-21: Centralized script/server utilities import — good alignment with the PR spec.Brings detect_default_script and select_servers_from_config into chat, ensuring consistent defaults across commands.
25-25: Settings injection for cleaner UX — good.Importing get_settings enables disabling progress display in REPL/listing paths.
149-153: Smart server defaults wired correctly.select_servers_from_config honors --servers, then dynamic URL/stdio, then config — matches the PR policy.
159-164: Nice: progress display disabled in listing mode via settings override.
221-225: Consistent UX: progress off for multi-model REPL.
249-253: LLM creation uses resolved servers — good.
298-301: /tools (parallel REPL) now prefers resolved servers — good.
318-321: /resources (parallel REPL) now prefers resolved servers — good.
392-395: One-shot multi-model path passes resolved servers — good.
420-425: REPL: settings override + progress off + load_user_app — good pattern.
445-449: Single-model REPL wired to resolved servers — good.
503-509: /model hot-swap preserves resolved server selection — good.
529-535: /tools now uses resolved servers when no explicit server — good.
549-556: /resources now uses resolved servers when no explicit server — good.
586-592: Past inconsistency fixed: /prompt now uses resolved_server_list.This addresses the previous review’s concern about ignoring dynamic/configured servers in /prompt.
729-733: One-shot single-model path passes resolved servers — good.
| server_names=resolved_server_list or target_servers, | ||
| context=app_obj.context, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor --server filter in Agent.server_names.
If resolved_server_list is non-empty, it currently overrides a provided --server. Prefer the explicit filter.
Apply:
- server_names=resolved_server_list or target_servers,
+ server_names=([server] if server else (resolved_server_list or target_servers)),📝 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.
| server_names=resolved_server_list or target_servers, | |
| context=app_obj.context, | |
| ) | |
| server_names=([server] if server else (resolved_server_list or target_servers)), | |
| context=app_obj.context, | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/commands/chat.py around lines 181-183, the server_names
assignment currently uses "resolved_server_list or target_servers", which lets
the resolved list override an explicit --server filter; change the logic so the
explicit target_servers (the --server filter) takes precedence by using
"target_servers or resolved_server_list" (or equivalent truthy check), ensuring
that when a --server value is provided it is honoured, falling back to
resolved_server_list only when target_servers is empty/None.
devmcp_agent.config.yamlMotivation
What changed
CLI layout (
src/mcp_agent/cli/main.py,src/mcp_agent/cli/__main__.py)init,config,quickstart,doctor,cloud, plus aliasesdeploy,loginmcp-agent devdev start(replaces olddev)dev chat,dev invoke,dev serve,dev go,dev server,dev build,dev logs,dev check,dev keys,dev models,dev clientmcp-agent devwith no subcommand behaves likedev startmcp-agent go …route todev go …Smart server defaults
--serversprovided ⇒ use itmcp_agent.config.yamlchat,go,invokeso flags are optional for common casesInit UX (
src/mcp_agent/cli/commands/init.py)main.py(asks to overwrite or choose another name)uv run <file>mcp-agent dev start --script <file>mcp-agent dev serve --script <file>mcp-agent config editClient helpers
mcp-agent dev client …(same functionality asconfiguremodule, clearer noun)Small docs/help alignments
doctorexample updated tomcp-agent dev start --script main.pymain.pyfirstNew helpers
detect_default_script(explicit)insrc/mcp_agent/cli/core/utils.pyselect_servers_from_config(servers_csv, url_servers, stdio_servers)in the same moduleBreaking/behavior notes
dev; usedev start,dev serve, etc.goremains available viadev go; top-levelgois routed for convenience.main.py; existingagent.pyprojects continue to work.How to test
Scaffold and run
mcp-agent init mcp-agent dev start # auto-detects main.py mcp-agent dev serve --transport http --port 8080Smart server defaults
Invoke agent/workflow
Serve test
Files touched (high level)
src/mcp_agent/cli/main.py(layout, groups, aliases)src/mcp_agent/cli/__main__.py(routing convenience)src/mcp_agent/cli/core/utils.py(new helpers)src/mcp_agent/cli/commands/dev.py(auto-detect script)src/mcp_agent/cli/commands/serve.py(auto-detect script; improved messages; test)src/mcp_agent/cli/commands/chat.py,go.py,invoke.py(auto-detect script; smart server defaults)src/mcp_agent/cli/commands/init.py(write tomain.py; improved next steps)src/mcp_agent/cli/commands/doctor.py(example alignment)Follow-ups (optional)
clientalias (in addition todev client) if you want higher discoverability-sshort flag for--scriptacross chat/go/invoke for parity with servedevumbrella and examplesSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation