Add cloud servers #420
Merged
Add cloud servers #420
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
38-66: Check permissions before prompting; fail fast.Move the permission check above the confirmation prompt to avoid asking users to confirm actions they cannot perform.
server_name = get_server_name(server) server_id = get_server_id(server) - if not force: + # Check permissions before any prompt + if isinstance(server, MCPApp): + can_delete = run_async(client.can_delete_app(server_id)) + else: + can_delete = run_async(client.can_delete_app_configuration(server_id)) + if not can_delete: + raise CLIError( + f"You do not have permission to delete this {server_type.lower()}. " + f"You can only delete servers that you created." + ) + + if not force: console.print( Panel( f"Name: [cyan]{server_name}[/cyan]\n" f"Type: [cyan]{server_type}[/cyan]\n" f"ID: [cyan]{server_id}[/cyan]\n\n" f"[bold red]⚠️ This action cannot be undone![/bold red]", title="Server to Delete", border_style="red", expand=False, ) ) confirm = typer.confirm(f"\nAre you sure you want to delete this {server_type.lower()}?") if not confirm: print_info("Deletion cancelled.") return - - if isinstance(server, MCPApp): - can_delete = run_async(client.can_delete_app(server_id)) - else: - can_delete = run_async(client.can_delete_app_configuration(server_id)) - - if not can_delete: - raise CLIError( - f"You do not have permission to delete this {server_type.lower()}. " - f"You can only delete servers that you created." - )src/mcp_agent/cli/core/utils.py (1)
29-41: Tighten identifier parsing (avoid accidental misclassification)Current fallback treats any string as an app_id. Consider stricter validation (prefix + simple pattern) to reduce surprises. Example:
def parse_app_identifier(identifier: str) -> Tuple[Optional[str], Optional[str], Optional[str]]: """Parse app identifier to extract app ID, config ID, and server URL.""" - + identifier = identifier.strip() if identifier.startswith(('http://', 'https://')): return None, None, identifier if identifier.startswith('apcnf_'): return None, identifier, None if identifier.startswith('app_'): return identifier, None, None return identifier, None, Nonesrc/mcp_agent/cli/cloud/commands/servers/describe/main.py (5)
21-30: Rename CLI option var to avoid shadowing built-in and keep usage consistentRename parameter to output_format and update call sites.
-@handle_server_api_errors -def describe_server( - id_or_url: str = typer.Argument(..., help="Server ID or URL to describe"), - format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), -) -> None: +@handle_server_api_errors +def describe_server( + id_or_url: str = typer.Argument(..., help="Server ID or URL to describe"), + output_format: str = typer.Option("text", "--format", help="Output format (text|json|yaml)"), +) -> None: """Describe a specific MCP Server.""" - validate_output_format(format) + validate_output_format(output_format) client = setup_authenticated_client() server = resolve_server(client, id_or_url) - print_server_description(server, format) + print_server_description(server, output_format)
35-38: De-duplicate format validation logicReuse the shared validator instead of a local list.
- valid_formats = ["text", "json", "yaml"] - if output_format not in valid_formats: - raise CLIError(f"Invalid format '{output_format}'. Valid options are: {', '.join(valid_formats)}") + validate_output_format(output_format)
62-69: Normalize name and URL fields in dict outputEnsure human-friendly defaults.
- server_name = server.name + server_name = server.name or "Unnamed" @@ - server_url = server_info.serverUrl if server_info else None + server_url = (server_info.serverUrl if server_info else None) or NoneAlso applies to: 81-97
104-111: Text renderer: default name and URL for better UXAvoid printing “None”.
- server_name = server.name + server_name = server.name or "Unnamed" @@ - if server_info: + if server_info: status_text = _server_status_text(server_info.status) - server_url = server_info.serverUrl + server_url = server_info.serverUrl or "N/A"Also applies to: 119-131, 136-138
157-164: Deduplicate status-to-emoji helper across commandsThis duplicates logic in servers/list. Consider centralizing a single helper in servers/utils to avoid drift.
src/mcp_agent/cli/cloud/commands/servers/utils.py (2)
101-103: Remove no-op replace in error wrapperThe second replace does nothing.
- func_name = func.__name__.replace('_', ' ').replace('server', 'server') + func_name = func.__name__.replace('_', ' ')
48-50: Consider exposing valid formats once to avoid driftMove the formats list to a module-level constant and reuse in describe/list/delete for consistency.
src/mcp_agent/cli/cloud/commands/servers/list/main.py (6)
23-27: Avoid shadowing built-ins (filter, format) and validate limit.Rename params to reduce confusion; also add a min bound for limit.
-def list_servers( - limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"), - filter: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"), - sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"), - format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), -) -> None: +def list_servers( + limit: Optional[int] = typer.Option(None, "--limit", min=1, help="Maximum number of results to return"), + filter_expr: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"), + sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"), + output_format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), +) -> None:- validate_output_format(format) + validate_output_format(output_format)- filtered_deployed = _apply_filter(list_apps_res.apps, filter) if filter else list_apps_res.apps - filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter) if filter else list_app_configs_res.appConfigurations + filtered_deployed = _apply_filter(list_apps_res.apps, filter_expr) if filter_expr else list_apps_res.apps + filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter_expr) if filter_expr else list_app_configs_res.appConfigurations- if format == "json": + if output_format == "json": _print_servers_json(sorted_deployed, sorted_configured) - elif format == "yaml": + elif output_format == "yaml": _print_servers_yaml(sorted_deployed, sorted_configured) else: - _print_servers_text(sorted_deployed, sorted_configured, filter, sort_by) + _print_servers_text(sorted_deployed, sorted_configured, filter_expr, sort_by)Also applies to: 43-43, 58-69
89-93: Prefer explicit type checks for configurations.Use
isinstance(..., MCPAppConfiguration)instead of attribute probing.- elif hasattr(server, 'app'): # MCPAppConfiguration + elif isinstance(server, MCPAppConfiguration): # MCPAppConfiguration name = server.app.name if server.app else "" description = server.app.description if server.app else "" status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE"- elif hasattr(server, 'app'): # MCPAppConfiguration + elif isinstance(server, MCPAppConfiguration): # MCPAppConfiguration name = server.app.name if server.app else "" created_at = server.createdAt status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE"Also applies to: 134-138
179-179: Use Rich’s rule instead of a fixed 80-char line.Adapts to terminal width automatically.
- console.print("\n" + "─" * 80 + "\n") + console.rule()
218-218: Safer YAML emission.Prefer
safe_dumpand preserve key order.- print(yaml.dump(output, default_flow_style=False)) + print(yaml.safe_dump(output, default_flow_style=False, sort_keys=False))
49-53: Consider pagination to honor large result sets.Currently only the first page (max_results) is fetched; iterate page tokens to collect all results when limit is None or above a page.
Would you like a follow-up patch adding paginated fetch for both APIs?
328-335: Deduplicate status rendering.
_server_status_textduplicates logic fromservers/describe/main.py. Move a single implementation toservers/utils.pyand reuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/mcp_agent/cli/cloud/commands/auth/logout/main.py(0 hunks)src/mcp_agent/cli/cloud/commands/auth/whoami/main.py(0 hunks)src/mcp_agent/cli/cloud/commands/logger/tail/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/logger/utils.py(0 hunks)src/mcp_agent/cli/cloud/commands/servers/__init__.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/delete/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/describe/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/list/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/utils.py(1 hunks)src/mcp_agent/cli/cloud/main.py(3 hunks)src/mcp_agent/cli/core/utils.py(2 hunks)
💤 Files with no reviewable changes (3)
- src/mcp_agent/cli/cloud/commands/auth/logout/main.py
- src/mcp_agent/cli/cloud/commands/logger/utils.py
- src/mcp_agent/cli/cloud/commands/auth/whoami/main.py
🧰 Additional context used
🧬 Code graph analysis (8)
src/mcp_agent/cli/cloud/commands/servers/utils.py (5)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/core/api_client.py (1)
UnauthenticatedError(9-12)src/mcp_agent/cli/core/utils.py (2)
parse_app_identifier(29-41)run_async(11-26)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/api_client.py (2)
MCPAppConfiguration(33-38)get_app_configuration(181-222)
src/mcp_agent/cli/cloud/commands/servers/delete/main.py (5)
src/mcp_agent/cli/core/utils.py (1)
run_async(11-26)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/cloud/commands/servers/utils.py (5)
setup_authenticated_client(18-36)resolve_server(53-77)handle_server_api_errors(80-105)get_server_name(108-120)get_server_id(123-135)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)src/mcp_agent/cli/mcp_app/api_client.py (3)
delete_app_configuration(477-510)can_delete_app(546-566)can_delete_app_configuration(568-588)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (1)
src/mcp_agent/cli/core/utils.py (2)
parse_app_identifier(29-41)resolve_server_url(44-106)
src/mcp_agent/cli/cloud/commands/servers/__init__.py (4)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)
list_servers(22-69)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (1)
describe_server(21-29)src/mcp_agent/cli/core/api_client.py (1)
delete(114-130)src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
delete_server(19-77)
src/mcp_agent/cli/core/utils.py (3)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/auth/models.py (1)
UserCredentials(10-64)src/mcp_agent/cli/core/api_client.py (2)
post(75-87)get(103-112)
src/mcp_agent/cli/cloud/commands/servers/describe/main.py (4)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/api_client.py (1)
MCPAppConfiguration(33-38)src/mcp_agent/cli/cloud/commands/servers/utils.py (5)
setup_authenticated_client(18-36)validate_output_format(39-50)resolve_server(53-77)handle_server_api_errors(80-105)clean_server_status(138-152)src/mcp_agent/cli/cloud/commands/servers/list/main.py (2)
_server_to_dict(221-233)_server_status_text(328-335)
src/mcp_agent/cli/cloud/main.py (3)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)
list_servers(22-69)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (1)
describe_server(21-29)src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
delete_server(19-77)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (5)
src/mcp_agent/cli/core/utils.py (1)
run_async(11-26)src/mcp_agent/cli/mcp_app/api_client.py (2)
MCPAppConfiguration(33-38)list_app_configurations(409-442)src/mcp_agent/cli/cloud/commands/servers/utils.py (4)
setup_authenticated_client(18-36)validate_output_format(39-50)handle_server_api_errors(80-105)clean_server_status(138-152)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (2)
_server_to_dict(59-97)_server_status_text(157-164)
🔇 Additional comments (8)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (1)
21-21: Verified: no stalelogger/utilsimports and URL resolution logic remains identical for streaming and one-off fetch paths.src/mcp_agent/cli/cloud/main.py (2)
25-29: Server commands import wiring is correct.
196-198: ‘apps’ alias under cloud is unique; no conflicting Typer named “apps” found.src/mcp_agent/cli/cloud/commands/servers/__init__.py (1)
1-11: Clean re-exports with all — looks good.src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
66-66: delete_app implementation confirmed:MCPAppClient.delete_app(app_id)parses the API response forappId, raises if missing, and returns the deleted app ID.src/mcp_agent/cli/core/utils.py (1)
7-7: Import from mcp_agent.cli.auth is correct
UserCredentials is re-exported in src/mcp_agent/cli/auth/init.py, so the existing import is valid.src/mcp_agent/cli/cloud/commands/servers/utils.py (1)
69-75: URL resolution should try both App and AppConfigurationA server URL may belong to either. Try app first, then fall back.
- if server_url: - return run_async(client.get_app(server_url=server_url)) + if server_url: + try: + return run_async(client.get_app(server_url=server_url)) + except Exception: + # Fall back to configuration by server URL + return run_async(client.get_app_configuration(server_url=server_url))Please confirm expected exception types for a non-matching URL so we can narrow the except clause (e.g., ValueError vs HTTP error). If UnauthenticatedError can be raised here, we should not swallow it.
src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)
49-56: Parallel fetch + client-side filter/sort look good.Good use of
asyncio.gather,run_async, and resilient client-side ops.Also applies to: 57-63
Comment on lines
+55
to
+56
| server_data = _server_to_dict(server) | ||
| print(yaml.dump(server_data, default_flow_style=False)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer safe_dump for YAML
Avoid emitting arbitrary Python tags.
- print(yaml.dump(server_data, default_flow_style=False))
+ print(yaml.safe_dump(server_data, default_flow_style=False))📝 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.
Suggested change
| server_data = _server_to_dict(server) | |
| print(yaml.dump(server_data, default_flow_style=False)) | |
| server_data = _server_to_dict(server) | |
| print(yaml.safe_dump(server_data, default_flow_style=False)) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/describe/main.py around lines 55-56,
the code uses yaml.dump which can emit arbitrary Python tags; replace
yaml.dump(server_data, default_flow_style=False) with
yaml.safe_dump(server_data, default_flow_style=False) so PyYAML uses the safe
serializer (and confirm yaml is imported from PyYAML); this prevents emitting
Python-specific tags and ensures only standard YAML types are produced.
| clean_server_status, | ||
| ) | ||
| from mcp_agent.cli.utils.ux import console, print_info | ||
| from datetime import datetime |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make 'created' sorting robust across tz-naive/aware datetimes.
Mixing naive/aware datetimes can raise TypeError and disable sorting. Normalize to epoch seconds.
-from datetime import datetime
+from datetime import datetime, timezone- elif sort_field_lower in ['created', 'created_at', 'date']:
- return created_at or datetime.min.replace(tzinfo=created_at.tzinfo if created_at else None)
+ elif sort_field_lower in ['created', 'created_at', 'date']:
+ if created_at:
+ if created_at.tzinfo is None:
+ created_at = created_at.replace(tzinfo=timezone.utc)
+ return created_at.timestamp()
+ return float('-inf')Also applies to: 151-153
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/list/main.py around line 18 and also
at lines 151-153, the sort on the "created" field must handle mixing tz-naive
and tz-aware datetimes; normalize each datetime to UTC and convert to epoch
seconds for comparison (if dt.tzinfo is None, treat as UTC via
replace(tzinfo=timezone.utc); otherwise astimezone(timezone.utc)), then use
dt.timestamp() as the sort key so sorting won’t raise TypeError for naive vs
aware datetimes.
Comment on lines
+236
to
+249
| def _server_config_to_dict(config: MCPAppConfiguration) -> dict: | ||
| """Convert MCPAppConfiguration to dictionary.""" | ||
| status_raw = config.appServerInfo.status if config.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| return { | ||
| "config_id": config.appConfigurationId, | ||
| "app_id": config.app.appId if config.app else None, | ||
| "name": config.app.name if config.app else "Unnamed", | ||
| "description": config.app.description if config.app else None, | ||
| "status": clean_server_status(status_raw), | ||
| "server_url": config.appServerInfo.serverUrl if config.appServerInfo else None, | ||
| "creator_id": config.creatorId, | ||
| "created_at": config.createdAt.isoformat() if config.createdAt else None, | ||
| "type": "configured" | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align JSON/YAML schema with describe output (consistent id).
describe uses "id" for both types; here configs expose "config_id". Unify to "id" (optionally keep "config_id" as alias).
return {
- "config_id": config.appConfigurationId,
+ "id": config.appConfigurationId,
+ "config_id": config.appConfigurationId, # alias
"app_id": config.app.appId if config.app else None,
"name": config.app.name if config.app else "Unnamed",
"description": config.app.description if config.app else None,
"status": clean_server_status(status_raw),
"server_url": config.appServerInfo.serverUrl if config.appServerInfo else None,
"creator_id": config.creatorId,
"created_at": config.createdAt.isoformat() if config.createdAt else None,
"type": "configured"
}🏁 Script executed:
#!/bin/bash
set -euxo pipefail
echo "1) List available subcommands under servers"
ls -1 src/mcp_agent/cli/cloud/commands/servers
echo "2) Locate any describe command files"
find src/mcp_agent/cli/cloud/commands/servers -type f | grep -i describe || true
echo "3) Inspect key mappings in servers/describe/main.py"
grep -R "def _server_to_dict" -n src/mcp_agent/cli/cloud/commands/servers/describe/main.py -C3 || true
echo "4) Search within describe for JSON keys (e.g., 'id', 'config_id')"
grep -R '"id"' -n src/mcp_agent/cli/cloud/commands/servers/describe/main.py -C2
grep -R '"config_id"' -n src/mcp_agent/cli/cloud/commands/servers/describe/main.py -C2 || trueLength of output: 1407
Align list output schema with describe command
In src/mcp_agent/cli/cloud/commands/servers/list/main.py, replace the "config_id" field with "id"—and retain "config_id" as an alias if backward-compatibility is needed.
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/list/main.py around lines 236 to
249, the output dictionary uses "config_id" but the describe command expects
"id"; change the dict to expose "id": config.appConfigurationId while also
preserving "config_id" as an alias for backward compatibility (i.e., include
both keys pointing to the same value), and ensure any tests or callers that
expect "config_id" continue to work by keeping that key.
Comment on lines
+290
to
+291
| console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") | ||
|
|
There was a problem hiding this comment.
Fix: guard None createdAt to avoid runtime crash.
server.createdAt can be None; calling strftime will raise.
- console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}")
+ console.print(f" Created: {(server.createdAt.strftime('%Y-%m-%d %H:%M:%S') if server.createdAt else 'Unknown')}")📝 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.
Suggested change
| console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") | |
| console.print(f" Created: {(server.createdAt.strftime('%Y-%m-%d %H:%M:%S') if server.createdAt else 'Unknown')}") |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/list/main.py around lines 290-291,
the code calls server.createdAt.strftime(...) which will crash when createdAt is
None; guard this by checking if server.createdAt is truthy before calling
strftime and otherwise print a fallback (e.g., "N/A" or an empty string), or use
a conditional expression to format createdAt only when not None so the CLI won't
raise an exception.
Comment on lines
+143
to
+156
| # Sub-typer for `mcp-agent servers` commands | ||
| app_cmd_servers = typer.Typer( | ||
| help="Management commands for MCP Servers", | ||
| no_args_is_help=True, | ||
| cls=HelpfulTyperGroup, | ||
| ) | ||
| app_cmd_servers.command(name="list")(list_servers) | ||
| app_cmd_servers.command(name="describe")(describe_server) | ||
| app_cmd_servers.command(name="delete")(delete_server) | ||
| app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers") | ||
|
|
||
| # Alias for servers - apps should behave identically | ||
| app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)") | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Duplicate top-level 'apps' command group; conflicts with existing apps group.
app.add_typer(..., name="apps", ...) is already used for the legacy apps group (Line 121). Adding another Typer under the same name will cause a Click/Typer command collision or overwrite.
Apply this diff to remove the conflicting alias at the top level (keep the dedicated servers group and the alias under cloud only):
app_cmd_servers.command(name="delete")(delete_server)
app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers")
-
-# Alias for servers - apps should behave identically
-app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)")Confirm duplicates:
🏁 Script executed:
#!/bin/bash
# Show all top-level add_typer calls that name "apps"
rg -nP 'app\.add_typer\([^,]+,\s*name="apps"' -C2Length of output: 754
Remove conflicting apps alias
The apps group is already registered at line 121 with app_cmd_apps; the additional alias at lines 154–155 causes a command collision. Remove:
-# Alias for servers - apps should behave identically
-app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)")[src/mcp_agent/cli/cloud/main.py:154–155]
📝 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.
Suggested change
| # Sub-typer for `mcp-agent servers` commands | |
| app_cmd_servers = typer.Typer( | |
| help="Management commands for MCP Servers", | |
| no_args_is_help=True, | |
| cls=HelpfulTyperGroup, | |
| ) | |
| app_cmd_servers.command(name="list")(list_servers) | |
| app_cmd_servers.command(name="describe")(describe_server) | |
| app_cmd_servers.command(name="delete")(delete_server) | |
| app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers") | |
| # Alias for servers - apps should behave identically | |
| app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)") | |
| # Sub-typer for `mcp-agent servers` commands | |
| app_cmd_servers = typer.Typer( | |
| help="Management commands for MCP Servers", | |
| no_args_is_help=True, | |
| cls=HelpfulTyperGroup, | |
| ) | |
| app_cmd_servers.command(name="list")(list_servers) | |
| app_cmd_servers.command(name="describe")(describe_server) | |
| app_cmd_servers.command(name="delete")(delete_server) | |
| app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers") |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/main.py around lines 143 to 156, there is a duplicate
registration: the app.add_typer(app_cmd_servers, name="apps", ...) at lines
154–155 conflicts with the previously registered app_cmd_apps at line 121;
remove the duplicate alias lines (154–155) so only the original apps typer
(app_cmd_apps) is registered, leaving the servers typer registration intact and
avoiding the command collision.
Comment on lines
+79
to
+88
| try: | ||
| async with httpx.AsyncClient(timeout=30.0) as client: | ||
| response = await client.post(f"{api_base}{endpoint}", json=payload, headers=headers) | ||
|
|
||
| if response.status_code == 404: | ||
| raise CLIError(not_found_msg) | ||
| elif response.status_code != 200: | ||
| raise CLIError(f"{api_error_msg}: {response.status_code} {response.text}") | ||
|
|
||
| data = response.json() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden HTTP/JSON error handling and unauthorized responses
- Add explicit 401/403 handling for a consistent “login” message.
- Guard against invalid JSON bodies to avoid unhandled exceptions.
try:
async with httpx.AsyncClient(timeout=30.0) as client:
response = await client.post(f"{api_base}{endpoint}", json=payload, headers=headers)
- if response.status_code == 404:
+ if response.status_code == 404:
raise CLIError(not_found_msg)
- elif response.status_code != 200:
+ elif response.status_code in (401, 403):
+ raise CLIError("Invalid API key. Run 'mcp-agent login' or set MCP_API_KEY environment variable with a new API key.")
+ elif response.status_code != 200:
raise CLIError(f"{api_error_msg}: {response.status_code} {response.text}")
- data = response.json()
+ try:
+ data = response.json()
+ except ValueError:
+ raise CLIError(f"{api_error_msg}: Invalid JSON response")
resource_info = data.get(response_key, {})
server_info = resource_info.get("appServerInfo")Also applies to: 83-87, 88-99, 105-106
🤖 Prompt for AI Agents
In src/mcp_agent/cli/core/utils.py around lines 79-88 (and similarly apply to
83-87, 88-99, 105-106), the HTTP/JSON handling must explicitly treat 401/403 as
an authentication/login error and guard against invalid JSON bodies; update the
code so that after receiving the response you first check for status codes 401
or 403 and raise CLIError with a consistent login message, then handle 404 and
other non-200 statuses as before, and finally parse JSON inside a try/except
that catches JSON decoding errors (e.g., ValueError/JSONDecodeError or
httpx.DecodingError) and raises a CLIError with a clear message including
response.status_code and response.text; apply this same pattern to the other
indicated blocks to ensure consistent auth handling and robust JSON parsing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/mcp_agent/cli/cloud/commands/servers/utils.py (1)
89-105: Polish error messages: Title-case 'server' in function namesMatches prior feedback; makes messages like “Error in List Servers”.
- func_name = func.__name__.replace('_', ' ') + func_name = func.__name__.replace('_', ' ').replace('server', 'Server')src/mcp_agent/cli/cloud/commands/servers/list/main.py (3)
18-18: Fix created-date sorting: normalize tz and use epoch secondsAvoids TypeError when mixing tz-aware/naive datetimes; sorts reliably.
-from datetime import datetime +from datetime import datetime, timezone @@ - elif sort_field_lower in ['created', 'created_at', 'date']: - return created_at or datetime.min.replace(tzinfo=created_at.tzinfo if created_at else None) + elif sort_field_lower in ['created', 'created_at', 'date']: + if created_at: + # Treat naive as UTC; normalize aware to UTC; compare by epoch seconds + if created_at.tzinfo is None: + created_at = created_at.replace(tzinfo=timezone.utc) + else: + created_at = created_at.astimezone(timezone.utc) + return created_at.timestamp() + return float('-inf')Also applies to: 151-153
236-249: Align list output schema with describe: expose 'id' (keep 'config_id' as alias)Keeps parity with describe JSON while preserving backward-compat callers.
def _server_config_to_dict(config: MCPAppConfiguration) -> dict: @@ - return { - "config_id": config.appConfigurationId, + return { + "id": config.appConfigurationId, + "config_id": config.appConfigurationId, # alias for backward-compat "app_id": config.app.appId if config.app else None,
290-291: Fix crash: createdAt may be NoneCalling strftime on None raises AttributeError.
- console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") + console.print(f" Created: {(server.createdAt.strftime('%Y-%m-%d %H:%M:%S') if server.createdAt else 'Unknown')}")
🧹 Nitpick comments (5)
src/mcp_agent/cli/cloud/commands/servers/utils.py (3)
39-51: Make format validation case-insensitive; accept 'yml' aliasImproves UX; CLI flags often vary in case and users type yml.
-def validate_output_format(format: str) -> None: +def validate_output_format(format: str) -> None: @@ - valid_formats = ["text", "json", "yaml"] - if format not in valid_formats: - raise CLIError(f"Invalid format '{format}'. Valid options are: {', '.join(valid_formats)}") + valid_formats = {"text", "json", "yaml", "yml"} + fmt = (format or "").lower() + if fmt not in valid_formats: + raise CLIError(f"Invalid format '{format}'. Valid options are: text, json, yaml")
53-77: Simplify server resolution via API helperYou can delegate to the client’s get_app_or_config to reduce branching, keeping the same behavior for IDs and URLs.
-def resolve_server(client: MCPAppClient, id_or_url: str) -> Union[MCPApp, MCPAppConfiguration]: +def resolve_server(client: MCPAppClient, id_or_url: str) -> Union[MCPApp, MCPAppConfiguration]: @@ - try: - app_id, config_id, server_url = parse_app_identifier(id_or_url) - - if server_url: - return run_async(client.get_app(server_url=server_url)) - elif config_id: - return run_async(client.get_app_configuration(app_config_id=config_id)) - else: - return run_async(client.get_app(app_id=app_id)) - + try: + return run_async(client.get_app_or_config(id_or_url))
137-152: Optional: extend status mappingIf API adds statuses (e.g., STARTING, ERROR), consider mapping them for consistency in text/JSON.
src/mcp_agent/cli/cloud/commands/servers/list/main.py (2)
23-27: Avoid using built-ins as parameter names (filter, format)Prevents shadowing and improves readability; CLI flags remain unchanged.
-def list_servers( - limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"), - filter: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"), - sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"), - format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), -) -> None: +def list_servers( + limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"), + filter_param: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"), + sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"), + output_format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), +) -> None: @@ - validate_output_format(format) + validate_output_format(output_format) @@ - filtered_deployed = _apply_filter(list_apps_res.apps, filter) if filter else list_apps_res.apps - filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter) if filter else list_app_configs_res.appConfigurations + filtered_deployed = _apply_filter(list_apps_res.apps, filter_param) if filter_param else list_apps_res.apps + filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter_param) if filter_param else list_app_configs_res.appConfigurations @@ - if format == "json": + if output_format == "json": _print_servers_json(sorted_deployed, sorted_configured) - elif format == "yaml": + elif output_format == "yaml": _print_servers_yaml(sorted_deployed, sorted_configured) else: - _print_servers_text(sorted_deployed, sorted_configured, filter, sort_by) + _print_servers_text(sorted_deployed, sorted_configured, filter_param, sort_by)
46-48: Validate --limit (> 0) to prevent API misuseNegative/zero values shouldn’t hit the API.
- max_results = limit or 100 + if limit is not None and limit <= 0: + raise typer.BadParameter("--limit must be a positive integer") + max_results = limit or 100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/mcp_agent/cli/cloud/commands/servers/list/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/servers/utils.py (5)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/core/api_client.py (1)
UnauthenticatedError(9-12)src/mcp_agent/cli/core/utils.py (2)
parse_app_identifier(29-41)run_async(11-26)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/api_client.py (3)
MCPAppClient(107-588)MCPAppConfiguration(33-38)get_app_configuration(181-222)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (5)
src/mcp_agent/cli/core/utils.py (1)
run_async(11-26)src/mcp_agent/cli/mcp_app/api_client.py (2)
MCPAppConfiguration(33-38)list_app_configurations(409-442)src/mcp_agent/cli/cloud/commands/servers/utils.py (4)
setup_authenticated_client(18-36)validate_output_format(39-50)handle_server_api_errors(80-105)clean_server_status(138-152)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (2)
_server_to_dict(59-97)_server_status_text(157-164)
🔇 Additional comments (6)
src/mcp_agent/cli/cloud/commands/servers/utils.py (1)
108-121: LGTM: server name resolution handles both typessrc/mcp_agent/cli/cloud/commands/servers/list/main.py (5)
49-56: Parallel fetch via asyncio.gather: LGTM
73-111: Filter logic is defensive and clearCovers both deployed and configured shapes; good fallback handling.
293-326: Configs text view guards createdAt and reads cleanlyGood formatting and safety checks.
221-234: Deployed JSON dict looks goodConsistent with describe; includes isoformat and status mapping.
190-195: Nice UX touch with applied filters hintClear feedback on client-side ops.
Comment on lines
+1
to
+335
| import asyncio | ||
| import json | ||
| from typing import List, Optional, Union | ||
|
|
||
| import typer | ||
| import yaml | ||
| from rich.panel import Panel | ||
|
|
||
| from mcp_agent.cli.core.utils import run_async | ||
| from mcp_agent.cli.mcp_app.api_client import MCPApp, MCPAppConfiguration | ||
| from ..utils import ( | ||
| setup_authenticated_client, | ||
| validate_output_format, | ||
| handle_server_api_errors, | ||
| clean_server_status, | ||
| ) | ||
| from mcp_agent.cli.utils.ux import console, print_info | ||
| from datetime import datetime | ||
|
|
||
|
|
||
| @handle_server_api_errors | ||
| def list_servers( | ||
| limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"), | ||
| filter: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"), | ||
| sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"), | ||
| format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"), | ||
| ) -> None: | ||
| """List MCP Servers with optional filtering and sorting. | ||
| Examples: | ||
| # Filter servers containing 'api' | ||
| mcp-agent cloud servers list --filter api | ||
| # Sort by creation date (newest first) | ||
| mcp-agent cloud servers list --sort-by -created | ||
| # Filter active servers and sort by name | ||
| mcp-agent cloud servers list --filter active --sort-by name | ||
| # Get JSON output with filtering | ||
| mcp-agent cloud servers list --filter production --format json | ||
| """ | ||
| validate_output_format(format) | ||
| client = setup_authenticated_client() | ||
|
|
||
| # Use limit or default | ||
| max_results = limit or 100 | ||
|
|
||
| async def parallel_requests(): | ||
| return await asyncio.gather( | ||
| client.list_apps(max_results=max_results), | ||
| client.list_app_configurations(max_results=max_results), | ||
| ) | ||
|
|
||
| list_apps_res, list_app_configs_res = run_async(parallel_requests()) | ||
|
|
||
| # Apply client-side filtering and sorting | ||
| filtered_deployed = _apply_filter(list_apps_res.apps, filter) if filter else list_apps_res.apps | ||
| filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter) if filter else list_app_configs_res.appConfigurations | ||
|
|
||
| sorted_deployed = _apply_sort(filtered_deployed, sort_by) if sort_by else filtered_deployed | ||
| sorted_configured = _apply_sort(filtered_configured, sort_by) if sort_by else filtered_configured | ||
|
|
||
| if format == "json": | ||
| _print_servers_json(sorted_deployed, sorted_configured) | ||
| elif format == "yaml": | ||
| _print_servers_yaml(sorted_deployed, sorted_configured) | ||
| else: | ||
| _print_servers_text(sorted_deployed, sorted_configured, filter, sort_by) | ||
|
|
||
|
|
||
|
|
||
| def _apply_filter(servers: List[Union[MCPApp, MCPAppConfiguration]], filter_expr: str) -> List[Union[MCPApp, MCPAppConfiguration]]: | ||
| """Apply client-side filtering to servers.""" | ||
| if not filter_expr: | ||
| return servers | ||
|
|
||
| filtered_servers = [] | ||
| # Support basic filtering by name, status, description | ||
| filter_lower = filter_expr.lower() | ||
|
|
||
| for server in servers: | ||
| # Get server attributes for filtering | ||
| try: | ||
| if isinstance(server, MCPApp): | ||
| name = server.name or "" | ||
| description = server.description or "" | ||
| status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| elif hasattr(server, 'app'): # MCPAppConfiguration | ||
| name = server.app.name if server.app else "" | ||
| description = server.app.description if server.app else "" | ||
| status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| else: # Fallback for other types (like test mocks) | ||
| name = getattr(server, 'name', '') or "" | ||
| description = getattr(server, 'description', '') or "" | ||
| server_info = getattr(server, 'appServerInfo', None) | ||
| status = server_info.status if server_info else "APP_SERVER_STATUS_OFFLINE" | ||
| except Exception: | ||
| # Skip servers that can't be processed | ||
| continue | ||
|
|
||
| # Clean status for filtering | ||
| clean_status = clean_server_status(status).lower() | ||
|
|
||
| # Check if filter matches name, description, or status | ||
| if (filter_lower in name.lower() or | ||
| filter_lower in description.lower() or | ||
| filter_lower in clean_status): | ||
| filtered_servers.append(server) | ||
|
|
||
| return filtered_servers | ||
|
|
||
|
|
||
| def _apply_sort(servers: List[Union[MCPApp, MCPAppConfiguration]], sort_field: str) -> List[Union[MCPApp, MCPAppConfiguration]]: | ||
| """Apply client-side sorting to servers.""" | ||
| if not sort_field: | ||
| return servers | ||
|
|
||
| # Normalize sort field | ||
| sort_field_lower = sort_field.lower() | ||
| reverse = False | ||
|
|
||
| # Support reverse sorting with - prefix | ||
| if sort_field_lower.startswith('-'): | ||
| reverse = True | ||
| sort_field_lower = sort_field_lower[1:] | ||
|
|
||
| def get_sort_key(server): | ||
| try: | ||
| if isinstance(server, MCPApp): | ||
| name = server.name or "" | ||
| created_at = server.createdAt | ||
| status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| elif hasattr(server, 'app'): # MCPAppConfiguration | ||
| name = server.app.name if server.app else "" | ||
| created_at = server.createdAt | ||
| status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| else: # Fallback for other types (like test mocks) | ||
| name = getattr(server, 'name', '') or "" | ||
| created_at = getattr(server, 'createdAt', None) | ||
| server_info = getattr(server, 'appServerInfo', None) | ||
| status = server_info.status if server_info else "APP_SERVER_STATUS_OFFLINE" | ||
| except Exception: | ||
| # Return default values for sorting if server can't be processed | ||
| name = "" | ||
| created_at = None | ||
| status = "APP_SERVER_STATUS_OFFLINE" | ||
|
|
||
| if sort_field_lower == 'name': | ||
| return name.lower() | ||
| elif sort_field_lower in ['created', 'created_at', 'date']: | ||
| return created_at or datetime.min.replace(tzinfo=None if created_at is None else created_at.tzinfo) | ||
| elif sort_field_lower == 'status': | ||
| return clean_server_status(status).lower() | ||
| else: | ||
| # Default to name if sort field not recognized | ||
| return name.lower() | ||
|
|
||
| try: | ||
| return sorted(servers, key=get_sort_key, reverse=reverse) | ||
| except Exception: | ||
| # If sorting fails, return original list | ||
| return servers | ||
|
|
||
|
|
||
| def _print_servers_text(deployed_servers: List[MCPApp], configured_servers: List[MCPAppConfiguration], filter_param: Optional[str], sort_by: Optional[str]) -> None: | ||
| """Print servers in text format.""" | ||
| print_info_header() | ||
|
|
||
| # Display deployed servers | ||
| if deployed_servers: | ||
| num_servers = len(deployed_servers) | ||
| print_info(f"Found {num_servers} deployed server(s):") | ||
| print_servers(deployed_servers) | ||
| else: | ||
| console.print("\n[bold blue]🖥️ Deployed MCP Servers (0)[/bold blue]") | ||
| print_info("No deployed servers found.") | ||
|
|
||
| console.print("\n" + "─" * 80 + "\n") | ||
|
|
||
| # Display configured servers | ||
| if configured_servers: | ||
| num_configs = len(configured_servers) | ||
| print_info(f"Found {num_configs} configured server(s):") | ||
| print_server_configs(configured_servers) | ||
| else: | ||
| console.print("\n[bold blue]⚙️ Configured MCP Servers (0)[/bold blue]") | ||
| print_info("No configured servers found.") | ||
|
|
||
| if filter_param or sort_by: | ||
| console.print(f"\n[dim]Applied filters: filter={filter_param or 'None'}, sort-by={sort_by or 'None'}[/dim]") | ||
| filter_desc = f"filter='{filter_param}'" if filter_param else "filter=None" | ||
| sort_desc = f"sort-by='{sort_by}'" if sort_by else "sort-by=None" | ||
| print_info(f"Client-side {filter_desc}, {sort_desc}. Sort fields: name, created, status (-prefix for reverse).") | ||
|
|
||
|
|
||
| def _print_servers_json(deployed_servers: List[MCPApp], configured_servers: List[MCPAppConfiguration]) -> None: | ||
| """Print servers in JSON format.""" | ||
| deployed_data = [_server_to_dict(server) for server in deployed_servers] | ||
| configured_data = [_server_config_to_dict(config) for config in configured_servers] | ||
|
|
||
| output = { | ||
| "deployed_servers": deployed_data, | ||
| "configured_servers": configured_data | ||
| } | ||
| print(json.dumps(output, indent=2, default=str)) | ||
|
|
||
|
|
||
| def _print_servers_yaml(deployed_servers: List[MCPApp], configured_servers: List[MCPAppConfiguration]) -> None: | ||
| """Print servers in YAML format.""" | ||
| deployed_data = [_server_to_dict(server) for server in deployed_servers] | ||
| configured_data = [_server_config_to_dict(config) for config in configured_servers] | ||
|
|
||
| output = { | ||
| "deployed_servers": deployed_data, | ||
| "configured_servers": configured_data | ||
| } | ||
| print(yaml.dump(output, default_flow_style=False)) | ||
|
|
||
|
|
||
| def _server_to_dict(server: MCPApp) -> dict: | ||
| """Convert MCPApp to dictionary.""" | ||
| status_raw = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| return { | ||
| "id": server.appId, | ||
| "name": server.name or "Unnamed", | ||
| "description": server.description, | ||
| "status": clean_server_status(status_raw), | ||
| "server_url": server.appServerInfo.serverUrl if server.appServerInfo else None, | ||
| "creator_id": server.creatorId, | ||
| "created_at": server.createdAt.isoformat() if server.createdAt else None, | ||
| "type": "deployed" | ||
| } | ||
|
|
||
|
|
||
| def _server_config_to_dict(config: MCPAppConfiguration) -> dict: | ||
| """Convert MCPAppConfiguration to dictionary.""" | ||
| status_raw = config.appServerInfo.status if config.appServerInfo else "APP_SERVER_STATUS_OFFLINE" | ||
| return { | ||
| "config_id": config.appConfigurationId, | ||
| "app_id": config.app.appId if config.app else None, | ||
| "name": config.app.name if config.app else "Unnamed", | ||
| "description": config.app.description if config.app else None, | ||
| "status": clean_server_status(status_raw), | ||
| "server_url": config.appServerInfo.serverUrl if config.appServerInfo else None, | ||
| "creator_id": config.creatorId, | ||
| "created_at": config.createdAt.isoformat() if config.createdAt else None, | ||
| "type": "configured" | ||
| } | ||
|
|
||
|
|
||
|
|
||
|
|
||
| def print_info_header() -> None: | ||
| """Print a styled header explaining the following tables""" | ||
| console.print( | ||
| Panel( | ||
| "Deployed Servers: [cyan]MCP Servers which you have bundled and deployed, as a developer[/cyan]\n" | ||
| "Configured Servers: [cyan]MCP Servers which you have configured to use with your MCP clients[/cyan]", | ||
| title="MCP Servers", | ||
| border_style="blue", | ||
| expand=False, | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def print_servers(servers: List[MCPApp]) -> None: | ||
| """Print a list of deployed servers in a clean, copyable format.""" | ||
| console.print(f"\n[bold blue]🖥️ Deployed MCP Servers ({len(servers)})[/bold blue]") | ||
|
|
||
| for i, server in enumerate(servers): | ||
| if i > 0: | ||
| console.print() | ||
|
|
||
| status = _server_status_text( | ||
| server.appServerInfo.status | ||
| if server.appServerInfo | ||
| else "APP_SERVER_STATUS_OFFLINE" | ||
| ) | ||
|
|
||
| console.print(f"[bold cyan]{server.name or 'Unnamed'}[/bold cyan] {status}") | ||
| console.print(f" Server ID: {server.appId}") | ||
|
|
||
| if server.appServerInfo and server.appServerInfo.serverUrl: | ||
| console.print(f" Server URL: {server.appServerInfo.serverUrl}") | ||
|
|
||
| if server.description: | ||
| console.print(f" Description: {server.description}") | ||
|
|
||
| console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") | ||
|
|
||
|
|
||
| def print_server_configs(server_configs: List[MCPAppConfiguration]) -> None: | ||
| """Print a list of configured servers in a clean, copyable format.""" | ||
| console.print( | ||
| f"\n[bold blue]⚙️ Configured MCP Servers ({len(server_configs)})[/bold blue]" | ||
| ) | ||
|
|
||
| for i, config in enumerate(server_configs): | ||
| if i > 0: | ||
| console.print() | ||
|
|
||
| status = _server_status_text( | ||
| config.appServerInfo.status | ||
| if config.appServerInfo | ||
| else "APP_SERVER_STATUS_OFFLINE" | ||
| ) | ||
|
|
||
| console.print( | ||
| f"[bold cyan]{config.app.name if config.app else 'Unnamed'}[/bold cyan] {status}" | ||
| ) | ||
| console.print(f" Config ID: {config.appConfigurationId}") | ||
|
|
||
| if config.app: | ||
| console.print(f" Server ID: {config.app.appId}") | ||
| if config.app.description: | ||
| console.print(f" Description: {config.app.description}") | ||
|
|
||
| if config.appServerInfo and config.appServerInfo.serverUrl: | ||
| console.print(f" Server URL: {config.appServerInfo.serverUrl}") | ||
|
|
||
| if config.createdAt: | ||
| console.print( | ||
| f" Created: {config.createdAt.strftime('%Y-%m-%d %H:%M:%S')}" | ||
| ) | ||
|
|
||
|
|
||
| def _server_status_text(status: str) -> str: | ||
| """Convert server status code to emoji.""" | ||
| if status == "APP_SERVER_STATUS_ONLINE": | ||
| return "[green]🟢 Active[/green]" | ||
| elif status == "APP_SERVER_STATUS_OFFLINE": | ||
| return "[red]🔴 Offline[/red]" | ||
| else: | ||
| return "❓ Unknown" No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick repo checks to prevent regressions
Run to confirm there are no other unsafe strftime calls and schema consistency across commands.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find unsafe strftime on createdAt:"
rg -nP "createdAt\.strftime\(" -C2
echo "2) Ensure list uses 'id' consistently for configs:"
rg -nP '"config_id"\s*:' src/mcp_agent/cli/cloud/commands | sed -n '1,200p'
echo "3) Verify describe output uses 'id' only:"
rg -nP '"id"\s*:' src/mcp_agent/cli/cloud/commands/servers/describe -n -C2Length of output: 3416
Replace all strftime calls with ISO‐formatted timestamps
- In
src/mcp_agent/cli/cloud/commands/workflow/status/main.py(line 79), replaceworkflow_info.createdAt.strftime('%Y-%m-%d %H:%M:%S'). - In
src/mcp_agent/cli/cloud/commands/servers/list/main.py(lines 290, 324), replaceserver.createdAt.strftime(...)andconfig.createdAt.strftime(...). - In
src/mcp_agent/cli/cloud/commands/apps/list/main.py(lines 133, 167), replaceapp.createdAt.strftime(...)andconfig.createdAt.strftime(...).
Schema checks passed: list commands use "config_id" consistently and describe outputs use "id" only.
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/list/main.py around lines 290 and
324, replace the .strftime('%Y-%m-%d %H:%M:%S') calls with ISO-formatted
timestamps: call createdAt.isoformat() (guarding for None) when printing created
times in print_servers and print_server_configs so they emit ISO 8601 strings;
ensure you handle missing createdAt by printing None or a suitable placeholder
(as other functions use .isoformat() with a None check).
Comment on lines
+18
to
+36
| def setup_authenticated_client() -> MCPAppClient: | ||
| """Setup authenticated MCP App client. | ||
| Returns: | ||
| Configured MCPAppClient instance | ||
| Raises: | ||
| CLIError: If authentication fails | ||
| """ | ||
| effective_api_key = load_api_key_credentials() | ||
|
|
||
| if not effective_api_key: | ||
| raise CLIError( | ||
| "Must be logged in to access servers. Run 'mcp-agent login'." | ||
| ) | ||
|
|
||
| return MCPAppClient( | ||
| api_url=DEFAULT_API_BASE_URL, api_key=effective_api_key | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor MCP_API_KEY env var in client setup
Support the env var mentioned in downstream errors so CLI works without prior login.
+import os
from functools import wraps
from typing import Union
@@
- effective_api_key = load_api_key_credentials()
+ effective_api_key = os.getenv("MCP_API_KEY") or load_api_key_credentials()📝 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.
Suggested change
| def setup_authenticated_client() -> MCPAppClient: | |
| """Setup authenticated MCP App client. | |
| Returns: | |
| Configured MCPAppClient instance | |
| Raises: | |
| CLIError: If authentication fails | |
| """ | |
| effective_api_key = load_api_key_credentials() | |
| if not effective_api_key: | |
| raise CLIError( | |
| "Must be logged in to access servers. Run 'mcp-agent login'." | |
| ) | |
| return MCPAppClient( | |
| api_url=DEFAULT_API_BASE_URL, api_key=effective_api_key | |
| ) | |
| import os | |
| from functools import wraps | |
| from typing import Union | |
| def setup_authenticated_client() -> MCPAppClient: | |
| """Setup authenticated MCP App client. | |
| Returns: | |
| Configured MCPAppClient instance | |
| Raises: | |
| CLIError: If authentication fails | |
| """ | |
| effective_api_key = os.getenv("MCP_API_KEY") or load_api_key_credentials() | |
| if not effective_api_key: | |
| raise CLIError( | |
| "Must be logged in to access servers. Run 'mcp-agent login'." | |
| ) | |
| return MCPAppClient( | |
| api_url=DEFAULT_API_BASE_URL, api_key=effective_api_key | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/servers/utils.py around lines 18 to 36, the
client setup currently only uses load_api_key_credentials(); update it to first
check the MCP_API_KEY environment variable (e.g., os.getenv("MCP_API_KEY")) and
use that if present, otherwise fall back to load_api_key_credentials(); if no
key is found still, raise the CLIError with messaging that mentions both logging
in and the MCP_API_KEY env var; then construct and return the MCPAppClient with
the resolved api_key.
This was referenced Sep 5, 2025
jtcorbett
force-pushed
the
09-05-add_cloud_servers
branch
from
f6e5d35 to
7dc6cf6
Compare
saqadri
approved these changes
Sep 5, 2025
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/mcp_agent/cli/cloud/main.py (1)
154-156: Remove duplicate top-level 'apps' group alias (conflicts with existing apps group at Line 121)This creates a Click/Typer collision. Keep the dedicated
serversgroup, but drop the duplicate alias at the top level. The cloud-level alias (cloud apps) can remain.app_cmd_servers.command(name="delete")(delete_server) app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers") - -# Alias for servers - apps should behave identically -app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)")src/mcp_agent/cli/cloud/commands/servers/list/main.py (3)
18-18: Make 'created' sorting robust across tz-naive/aware datetimesNormalize to UTC and use epoch seconds; avoids TypeError and ensures consistent ordering.
-from datetime import datetime +from datetime import datetime, timezone @@ - elif sort_field_lower in ['created', 'created_at', 'date']: - return created_at or datetime.min.replace(tzinfo=None if created_at is None else created_at.tzinfo) + elif sort_field_lower in ['created', 'created_at', 'date']: + if created_at: + if created_at.tzinfo is None: + created_at = created_at.replace(tzinfo=timezone.utc) + else: + created_at = created_at.astimezone(timezone.utc) + return created_at.timestamp() + return float('-inf')Also applies to: 149-153
236-249: Align list JSON/YAML schema with describe output (idvsconfig_id)Expose
"id"for configurations (keep"config_id"as alias for BC).def _server_config_to_dict(config: MCPAppConfiguration) -> dict: @@ - return { - "config_id": config.appConfigurationId, + return { + "id": config.appConfigurationId, + "config_id": config.appConfigurationId, # alias for backward-compat "app_id": config.app.appId if config.app else None, "name": config.app.name if config.app else "Unnamed", "description": config.app.description if config.app else None, "status": clean_server_status(status_raw), "server_url": config.appServerInfo.serverUrl if config.appServerInfo else None, "creator_id": config.creatorId, "created_at": config.createdAt.isoformat() if config.createdAt else None, "type": "configured" }
290-291: Guard createdAt and prefer ISO 8601 formattingAvoid crashes when
createdAtis None and keep consistent formatting across outputs.- console.print(f" Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") + console.print(f" Created: {(server.createdAt.isoformat() if server.createdAt else 'Unknown')}") @@ - if config.createdAt: - console.print( - f" Created: {config.createdAt.strftime('%Y-%m-%d %H:%M:%S')}" - ) + console.print(f" Created: {(config.createdAt.isoformat() if config.createdAt else 'Unknown')}")Also applies to: 322-325
🧹 Nitpick comments (4)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (3)
21-22: Use run_async helper instead of asyncio.run for robustnessThis avoids RuntimeError when invoked under an already-running event loop (e.g., tests) and aligns with servers/list usage.
-from mcp_agent.cli.core.utils import parse_app_identifier, resolve_server_url +from mcp_agent.cli.core.utils import parse_app_identifier, resolve_server_url, run_async @@ - asyncio.run(_stream_logs( + run_async(_stream_logs( app_id=app_id, config_id=config_id, server_url=server_url, credentials=credentials, grep_pattern=grep, app_identifier=app_identifier, format=format, )) @@ - asyncio.run(_fetch_logs( + run_async(_fetch_logs( app_id=app_id, config_id=config_id, server_url=server_url, credentials=credentials, since=since, grep_pattern=grep, limit=limit, order_by=order_by, asc=asc, desc=desc, format=format, ))Also applies to: 133-157
379-383: Simplify Rich style markup; remove unsupported “not bold”Rich doesn’t support “not bold” in markup. Use dim for timestamps.
- console.print( - f"[bright_black not bold]{timestamp}[/bright_black not bold] " + console.print( + f"[dim]{timestamp}[/dim] " f"[{level_style}]{level:7}[/{level_style}] " f"{message}" )
409-413: Stream YAML as a single mapping, not a singleton listBetter ergonomics for piping/jq/yq.
- elif format == "yaml": - cleaned_entry = _clean_log_entry(log_entry) - print(yaml.dump([cleaned_entry], default_flow_style=False)) + elif format == "yaml": + cleaned_entry = _clean_log_entry(log_entry) + print(yaml.dump(cleaned_entry, default_flow_style=False))src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)
314-316: Rename label to “App ID” for configured serversFor configurations, this value is the underlying App ID, not a “Server ID”.
- if config.app: - console.print(f" Server ID: {config.app.appId}") + if config.app: + console.print(f" App ID: {config.app.appId}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/mcp_agent/cli/cloud/commands/auth/logout/main.py(0 hunks)src/mcp_agent/cli/cloud/commands/auth/whoami/main.py(0 hunks)src/mcp_agent/cli/cloud/commands/logger/tail/main.py(7 hunks)src/mcp_agent/cli/cloud/commands/logger/utils.py(0 hunks)src/mcp_agent/cli/cloud/commands/servers/__init__.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/delete/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/describe/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/list/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/utils.py(1 hunks)src/mcp_agent/cli/cloud/main.py(4 hunks)src/mcp_agent/cli/core/utils.py(2 hunks)
💤 Files with no reviewable changes (3)
- src/mcp_agent/cli/cloud/commands/auth/whoami/main.py
- src/mcp_agent/cli/cloud/commands/auth/logout/main.py
- src/mcp_agent/cli/cloud/commands/logger/utils.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/mcp_agent/cli/cloud/commands/servers/init.py
- src/mcp_agent/cli/cloud/commands/servers/describe/main.py
- src/mcp_agent/cli/cloud/commands/servers/delete/main.py
- src/mcp_agent/cli/cloud/commands/servers/utils.py
- src/mcp_agent/cli/core/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcp_agent/cli/cloud/main.py (3)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)
list_servers(22-69)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (1)
describe_server(21-29)src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
delete_server(19-77)
src/mcp_agent/cli/cloud/commands/servers/list/main.py (5)
src/mcp_agent/cli/core/utils.py (1)
run_async(11-26)src/mcp_agent/cli/mcp_app/api_client.py (2)
MCPAppConfiguration(33-38)list_app_configurations(409-442)src/mcp_agent/cli/cloud/commands/servers/utils.py (4)
setup_authenticated_client(18-36)validate_output_format(39-50)handle_server_api_errors(80-105)clean_server_status(138-152)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)src/mcp_agent/cli/cloud/commands/servers/describe/main.py (2)
_server_to_dict(59-97)_server_status_text(157-164)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (3)
src/mcp_agent/cli/core/utils.py (2)
parse_app_identifier(29-41)resolve_server_url(44-106)src/mcp_agent/cli/auth/main.py (1)
load_credentials(32-46)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)
🔇 Additional comments (1)
src/mcp_agent/cli/cloud/main.py (1)
143-153: Servers command group wiring looks goodRegistration of list/describe/delete under
serverswith HelpfulTyperGroup is clear.
Comment on lines
302
to
334
| buffer = "" | ||
| async for chunk in response.aiter_text(): | ||
| buffer += chunk | ||
| lines = buffer.split("\n") | ||
|
|
||
| lines = buffer.split('\n') | ||
| for line in lines[:-1]: | ||
| if line.startswith("data:"): | ||
| data_content = line.removeprefix("data:") | ||
|
|
||
| if line.startswith('data:'): | ||
| data_content = line.removeprefix('data:') | ||
| try: | ||
| log_data = json.loads(data_content) | ||
|
|
||
| if "message" in log_data: | ||
| timestamp = log_data.get("time") | ||
| if 'message' in log_data: | ||
| timestamp = log_data.get('time') | ||
| if timestamp: | ||
| formatted_timestamp = ( | ||
| _convert_timestamp_to_local(timestamp) | ||
| ) | ||
| formatted_timestamp = _convert_timestamp_to_local(timestamp) | ||
| else: | ||
| formatted_timestamp = datetime.now().isoformat() | ||
|
|
||
| log_entry = { | ||
| "timestamp": formatted_timestamp, | ||
| "message": log_data["message"], | ||
| "level": log_data.get("level", "INFO"), | ||
| 'timestamp': formatted_timestamp, | ||
| 'message': log_data['message'], | ||
| 'level': log_data.get('level', 'INFO') | ||
| } | ||
|
|
||
| if not grep_pattern or _matches_pattern( | ||
| log_entry["message"], grep_pattern | ||
| ): | ||
|
|
||
| if not grep_pattern or _matches_pattern(log_entry['message'], grep_pattern): | ||
| _display_log_entry(log_entry, format=format) | ||
|
|
||
| except json.JSONDecodeError: | ||
| # Skip malformed JSON | ||
| continue | ||
|
|
||
| except httpx.RequestError as e: |
There was a problem hiding this comment.
Fix streaming buffer growth and duplicate processing in SSE loop
buffer is never trimmed, so previously processed lines are re-processed on every chunk and memory grows unbounded. Keep only the last partial line after each pass.
async for chunk in response.aiter_text():
buffer += chunk
- lines = buffer.split('\n')
+ lines = buffer.split('\n')
for line in lines[:-1]:
if line.startswith('data:'):
data_content = line.removeprefix('data:')
try:
log_data = json.loads(data_content)
if 'message' in log_data:
timestamp = log_data.get('time')
if timestamp:
formatted_timestamp = _convert_timestamp_to_local(timestamp)
else:
formatted_timestamp = datetime.now().isoformat()
log_entry = {
'timestamp': formatted_timestamp,
'message': log_data['message'],
'level': log_data.get('level', 'INFO')
}
if not grep_pattern or _matches_pattern(log_entry['message'], grep_pattern):
_display_log_entry(log_entry, format=format)
except json.JSONDecodeError:
# Skip malformed JSON
continue
-
+ # retain only the last partial line in the buffer
+ buffer = lines[-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.
Suggested change
| buffer = "" | |
| async for chunk in response.aiter_text(): | |
| buffer += chunk | |
| lines = buffer.split("\n") | |
| lines = buffer.split('\n') | |
| for line in lines[:-1]: | |
| if line.startswith("data:"): | |
| data_content = line.removeprefix("data:") | |
| if line.startswith('data:'): | |
| data_content = line.removeprefix('data:') | |
| try: | |
| log_data = json.loads(data_content) | |
| if "message" in log_data: | |
| timestamp = log_data.get("time") | |
| if 'message' in log_data: | |
| timestamp = log_data.get('time') | |
| if timestamp: | |
| formatted_timestamp = ( | |
| _convert_timestamp_to_local(timestamp) | |
| ) | |
| formatted_timestamp = _convert_timestamp_to_local(timestamp) | |
| else: | |
| formatted_timestamp = datetime.now().isoformat() | |
| log_entry = { | |
| "timestamp": formatted_timestamp, | |
| "message": log_data["message"], | |
| "level": log_data.get("level", "INFO"), | |
| 'timestamp': formatted_timestamp, | |
| 'message': log_data['message'], | |
| 'level': log_data.get('level', 'INFO') | |
| } | |
| if not grep_pattern or _matches_pattern( | |
| log_entry["message"], grep_pattern | |
| ): | |
| if not grep_pattern or _matches_pattern(log_entry['message'], grep_pattern): | |
| _display_log_entry(log_entry, format=format) | |
| except json.JSONDecodeError: | |
| # Skip malformed JSON | |
| continue | |
| except httpx.RequestError as e: | |
| buffer = "" | |
| async for chunk in response.aiter_text(): | |
| buffer += chunk | |
| lines = buffer.split('\n') | |
| for line in lines[:-1]: | |
| if line.startswith('data:'): | |
| data_content = line.removeprefix('data:') | |
| try: | |
| log_data = json.loads(data_content) | |
| if 'message' in log_data: | |
| timestamp = log_data.get('time') | |
| if timestamp: | |
| formatted_timestamp = _convert_timestamp_to_local(timestamp) | |
| else: | |
| formatted_timestamp = datetime.now().isoformat() | |
| log_entry = { | |
| 'timestamp': formatted_timestamp, | |
| 'message': log_data['message'], | |
| 'level': log_data.get('level', 'INFO') | |
| } | |
| if not grep_pattern or _matches_pattern(log_entry['message'], grep_pattern): | |
| _display_log_entry(log_entry, format=format) | |
| except json.JSONDecodeError: | |
| # Skip malformed JSON | |
| continue | |
| # retain only the last partial line in the buffer | |
| buffer = lines[-1] | |
| except httpx.RequestError as e: |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/logger/tail/main.py around lines 302 to 334,
the SSE streaming loop appends chunks to buffer but never trims it, causing
already-processed lines to be re-processed and buffer to grow unbounded; after
splitting buffer into lines and handling lines[:-1], set buffer = lines[-1] (the
last partial line) so only the incomplete tail is retained between chunks,
ensuring processed lines are not duplicated and memory stays bounded.
Merge activity
|
This was referenced Sep 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR
Added new server management commands to the MCP Agent CLI, allowing users to list, describe, and delete MCP servers.
What changed?
logger/utils.pytocore/utils.pyfor broader reuseserverscommand group with several subcommands:list: Display deployed and configured servers with filtering and sorting optionsdescribe: Show detailed information about a specific serverdelete: Remove a server with confirmation promptappscommands mirror theserverscommandsHow to test?
Why make this change?
This change enhances the MCP Agent CLI with comprehensive server management capabilities, making it easier for users to monitor, manage, and troubleshoot their MCP servers. The new commands provide a consistent interface with multiple output formats, supporting both human-readable text and machine-parsable JSON/YAML for automation. The implementation follows best practices with proper error handling, input validation, and user-friendly output formatting.
Summary by CodeRabbit
New Features
Refactor
Chores