Allow logger to also take URLs #451
Conversation
WalkthroughRefactors CLI server resolution to use client.get_app_or_config and server objects (MCPApp/MCPAppConfiguration). Logger tail now fetches/streams logs based on a server object and server.appServerInfo.serverUrl. Removes parse_app_identifier, resolve_server_url, and resolve_server helpers from utils; updates servers and workflows commands accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI: logger tail
participant Client as MCPAppClient
participant Server as Server (MCPApp/MCPAppConfiguration)
participant LogsAPI as Logs API
User->>CLI: tail --app <identifier> [--since/--limit/...]
CLI->>Client: get_app_or_config(identifier)
Client-->>CLI: Server
alt Fetch once
CLI->>Client: get_app_logs(app_id/app_configuration_id, order_by, dir, since, limit)
Client-->>CLI: LogEntries
CLI->>CLI: Optional grep + format
CLI-->>User: Display logs (title=app_identifier)
else Stream
CLI->>CLI: server_url = Server.appServerInfo.serverUrl
CLI->>LogsAPI: GET {server_url}/logs/stream<br/>Headers: X-Routing-Key (deployment_id)
LogsAPI-->>CLI: SSE/stream events
CLI-->>User: Live log lines
end
sequenceDiagram
autonumber
actor User
participant CLI as CLI: workflows/*, servers/*
participant Client as MCPAppClient
participant Server as Server (MCPApp/MCPAppConfiguration)
User->>CLI: cmd --server <id-or-url>
alt Input is URL
CLI->>CLI: Use provided URL directly
else Identifier
CLI->>Client: get_app_or_config(id)
Client-->>CLI: Server
CLI->>CLI: Extract serverUrl = Server.appServerInfo.serverUrl
end
CLI-->>User: Proceed with action using serverUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jtcorbett
requested review from
andrew-lastmile,
petersonbill64 and
saqadri
jtcorbett
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/mcp_agent/cli/cloud/commands/workflows/list/main.py (1)
48-51: Fix potential double “//” in SSE URLUse rstrip('/') before appending /sse.
- sse_url = ( - f"{server_url}/sse" if not server_url.endswith("/sse") else server_url - ) + sse_url = ( + f"{server_url.rstrip('/')}/sse" + if not server_url.endswith("/sse") + else server_url + )src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (1)
48-51: Fix potential double “//” in SSE URLUse rstrip('/') when appending /sse.
- sse_url = ( - f"{server_url}/sse" if not server_url.endswith("/sse") else server_url - ) + sse_url = ( + f"{server_url.rstrip('/')}/sse" + if not server_url.endswith("/sse") + else server_url + )src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (1)
48-51: Fix potential double “//” in SSE URLUse rstrip('/') when appending.
- sse_url = ( - f"{server_url}/sse" if not server_url.endswith("/sse") else server_url - ) + sse_url = ( + f"{server_url.rstrip('/')}/sse" + if not server_url.endswith("/sse") + else server_url + )src/mcp_agent/cli/cloud/commands/logger/tail/main.py (2)
312-317: Fix SSE buffer growth and duplicate processing.
Currently, processed lines remain in buffer, causing unbounded growth and repeated parsing.buffer += chunk lines = buffer.split("\n") for line in lines[:-1]: if line.startswith("data:"): data_content = line.removeprefix("data:") + # Keep only the trailing partial line for the next iteration + buffer = lines[-1]
439-444: Handle ms/ISO timestamps to prevent crashes and wrong times.
SSE often uses ms or ISO strings; large ms values can raise or format far-future times.-def _convert_timestamp_to_local(timestamp: float) -> str: - """Convert UTC timestamp to local time ISO format.""" - dt_utc = datetime.fromtimestamp(timestamp, timezone.utc) - dt_local = dt_utc.astimezone() - return dt_local.isoformat() +def _convert_timestamp_to_local(timestamp: Any) -> str: + """Convert UTC/epoch(ms|s|ns) or ISO timestamp to local time ISO format.""" + try: + if isinstance(timestamp, str): + dt_utc = datetime.fromisoformat(timestamp.replace("Z", "+00:00")) + else: + t = float(timestamp) + if t > 1e12: # ns -> s + t /= 1e9 + elif t > 1e10: # ms -> s + t /= 1e3 + dt_utc = datetime.fromtimestamp(t, timezone.utc) + return dt_utc.astimezone().isoformat() + except Exception: + return datetime.now().astimezone().isoformat()
🧹 Nitpick comments (8)
src/mcp_agent/cli/core/utils.py (1)
12-27: run_async fallback is unsafe inside a running event loopCalling run_until_complete on an already running loop will raise RuntimeError. Prefer failing fast with a clear message; callers in async contexts should await directly.
Apply:
def run_async(coro): @@ - except RuntimeError as e: - # If we're already in an event loop (like in pytest-asyncio tests) - if "cannot be called from a running event loop" in str(e): - loop = asyncio.get_event_loop() - return loop.run_until_complete(coro) - raise + except RuntimeError as e: + if "cannot be called from a running event loop" in str(e): + raise RuntimeError( + "run_async cannot be used from within an async context; await the coroutine instead." + ) from e + raisesrc/mcp_agent/cli/cloud/commands/servers/describe/main.py (1)
22-27: Update help text to reflect URL supportInclude “or server URL”.
- id_or_url: str = typer.Argument( - ..., help="Server ID or app configuration ID to describe" - ), + id_or_url: str = typer.Argument( + ..., help="Server ID, app configuration ID, or server URL to describe" + ),src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (1)
263-264: Non-standard color “orange”Rich may not support [orange] everywhere; use [yellow] for portability.
- return "[orange]⏰ Timed Out[/orange]" + return "[yellow]⏰ Timed Out[/yellow]"src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
18-23: Update help text to reflect URL supportInclude URL in the prompt.
- id_or_url: str = typer.Argument( - ..., help="Server ID or app configuration ID to delete" - ), + id_or_url: str = typer.Argument( + ..., help="Server ID, app configuration ID, or server URL to delete" + ),src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (1)
106-111: Use shared validator for format for consistencyReuse validate_output_format to match other commands.
- if format not in ["text", "json", "yaml"]: - console.print("[red]Error: --format must be 'text', 'json', or 'yaml'[/red]") - raise typer.Exit(6) - - run_async(_describe_workflow_async(server_id_or_url, run_id, format)) + validate_output_format(format) + run_async(_describe_workflow_async(server_id_or_url, run_id, format))src/mcp_agent/cli/cloud/commands/logger/tail/main.py (3)
33-33: Docs/UX: help and examples now mention server URLs — nice.
Optional: explicitly show that the scheme is required (e.g., https://) in the help string.- help="App ID, app configuration ID, or server URL to retrieve logs for" + help="App ID, app configuration ID, or server URL (including scheme, e.g., https://...)"Also applies to: 96-97
186-193: Prefer isinstance over hasattr; add an explicit else guard.
Reduces ambiguity and makes failures obvious if a different type slips through.- if hasattr(server, 'appId'): # MCPApp - app_id = server.appId - config_id = None - else: # MCPAppConfiguration - app_id = None - config_id = server.appConfigurationId + if isinstance(server, MCPApp): + app_id = server.appId + config_id = None + elif isinstance(server, MCPAppConfiguration): + app_id = None + config_id = server.appConfigurationId + else: + raise CLIError(f"Unsupported server object type: {type(server).__name__}")
253-254: Nit: title now uses the raw identifier.
Optional: include a friendly name when available, e.g., server.app.name or the server URL hostname, falling back to the identifier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py(8 hunks)src/mcp_agent/cli/cloud/commands/servers/delete/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/servers/describe/main.py(2 hunks)src/mcp_agent/cli/cloud/commands/utils.py(0 hunks)src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/workflows/describe/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/workflows/list/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/workflows/resume/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/workflows/runs/main.py(1 hunks)src/mcp_agent/cli/core/utils.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/mcp_agent/cli/cloud/commands/utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/utils/config.py : Configuration values such as quality_threshold, max_refinement_attempts, consolidation_interval, and evaluator_model_provider must be loaded from mcp_agent.config.yaml.
Applied to files:
src/mcp_agent/cli/cloud/commands/logger/tail/main.py
🧬 Code graph analysis (8)
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/workflows/list/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/servers/describe/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/servers/delete/main.py (2)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (1)
get_app_or_config(247-283)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (4)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/api_client.py (3)
MCPApp(22-29)MCPAppConfiguration(33-38)get_app_or_config(247-283)src/mcp_agent/cli/cloud/commands/utils.py (1)
setup_authenticated_client(17-31)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)
🔇 Additional comments (7)
src/mcp_agent/cli/cloud/commands/servers/describe/main.py (1)
16-33: Good switch to client.get_app_or_config()Synchronous usage of run_async here is appropriate.
src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (1)
61-75: Don't forward --reason — backend tool doesn't accept itServer-side "workflows-cancel" (src/mcp_agent/server/app_server.py, ~lines 863–865) only accepts run_id/workflow_id; adding tool_params["reason"] would be unsupported. Either add a 'reason' parameter+handling in the server tool, or keep the CLI --reason without forwarding.
Likely an incorrect or invalid review comment.
src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)
27-28: Good: centralized resolution via get_app_or_configSynchronous run_async use is correct here.
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (4)
9-9: LGTM: imports and typing updates align with the refactor.Also applies to: 22-22, 24-24
144-149: LGTM: call sites now pass a resolved server object.Also applies to: 152-164
174-175: LGTM: server-typed signatures clarify intent.Also applies to: 182-183
257-262: LGTM: streamed variant matches the fetch signature.
| client = setup_authenticated_client() | ||
| server = run_async(client.get_app_or_config(app_identifier)) | ||
|
|
||
| try: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch resolution errors: wrap get_app_or_config in the existing try.
Right now, failures raise before the try/except and bypass clean CLIError formatting.
- client = setup_authenticated_client()
- server = run_async(client.get_app_or_config(app_identifier))
-
- try:
+ try:
+ client = setup_authenticated_client()
+ server = run_async(client.get_app_or_config(app_identifier))Optional (outside this hunk): add specific excepts for httpx.HTTPStatusError (401/404) around this try to mirror _fetch_logs.
📝 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.
| client = setup_authenticated_client() | |
| server = run_async(client.get_app_or_config(app_identifier)) | |
| try: | |
| try: | |
| client = setup_authenticated_client() | |
| server = run_async(client.get_app_or_config(app_identifier)) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/logger/tail/main.py around lines 137 to 140,
the call to client.get_app_or_config(app_identifier) is executed before entering
the try/except so any resolution errors escape the CLIError handling; move or
wrap the run_async(client.get_app_or_config(app_identifier)) invocation inside
the existing try block so its exceptions are caught and formatted correctly, and
optionally add specific except clauses for httpx.HTTPStatusError (handling
401/404) alongside the existing excepts to mirror _fetch_logs.
| # Get server URL directly from the server object | ||
| if not server.appServerInfo or not server.appServerInfo.serverUrl: | ||
| raise CLIError("Server URL not available - server may not be deployed") | ||
|
|
||
| server_url = server.appServerInfo.serverUrl | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden server URL handling; add connect timeout for SSE.
- Append /logs relative to any existing path (not just host root).
- Derive routing key robustly; hostname may be None or lack dots.
- Use a finite connect timeout for AsyncClient; keep read timeout None for streaming.
- if not server.appServerInfo or not server.appServerInfo.serverUrl:
+ if not getattr(server, "appServerInfo", None) or not server.appServerInfo.serverUrl:
raise CLIError("Server URL not available - server may not be deployed")
-
- server_url = server.appServerInfo.serverUrl
+ server_url = server.appServerInfo.serverUrl.rstrip("/")Additional changes outside this hunk to complete the fix:
- parsed = urlparse(server_url)
- stream_url = f"{parsed.scheme}://{parsed.netloc}/logs"
+ parsed = urlparse(server_url)
+ base_path = (parsed.path or "").rstrip("/")
+ stream_url = f"{parsed.scheme}://{parsed.netloc}{base_path}/logs"
- hostname = parsed.hostname or ""
- deployment_id = hostname.split(".")[0] if "." in hostname else hostname
+ hostname = (parsed.hostname or "").strip()
+ deployment_id = hostname.split(".", 1)[0] if hostname else ""And for the client (outside this hunk):
- async with httpx.AsyncClient(timeout=None) as client:
+ async with httpx.AsyncClient(timeout=httpx.Timeout(connect=10.0, read=None)) as client:📝 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.
| # Get server URL directly from the server object | |
| if not server.appServerInfo or not server.appServerInfo.serverUrl: | |
| raise CLIError("Server URL not available - server may not be deployed") | |
| server_url = server.appServerInfo.serverUrl | |
| # Get server URL directly from the server object | |
| if not getattr(server, "appServerInfo", None) or not server.appServerInfo.serverUrl: | |
| raise CLIError("Server URL not available - server may not be deployed") | |
| server_url = server.appServerInfo.serverUrl.rstrip("/") |
| server = run_async(client.get_app_or_config(server_id_or_url)) | ||
|
|
There was a problem hiding this comment.
Bug: run_async used inside async function
Await instead.
- server = run_async(client.get_app_or_config(server_id_or_url))
+ server = await client.get_app_or_config(server_id_or_url)📝 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 = run_async(client.get_app_or_config(server_id_or_url)) | |
| server = await client.get_app_or_config(server_id_or_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py around lines 27-28,
the code calls run_async(client.get_app_or_config(server_id_or_url)) inside an
async function; replace this with an await expression (await
client.get_app_or_config(server_id_or_url)) and remove run_async. Ensure the
enclosing function is declared async and that any callers properly await it or
handle the coroutine.
| server = run_async(client.get_app_or_config(server_id_or_url)) | ||
|
|
There was a problem hiding this comment.
Bug: run_async used inside async function
Await instead.
- server = run_async(client.get_app_or_config(server_id_or_url))
+ server = await client.get_app_or_config(server_id_or_url)📝 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 = run_async(client.get_app_or_config(server_id_or_url)) | |
| server = await client.get_app_or_config(server_id_or_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/describe/main.py around lines
29-30, the code calls run_async(client.get_app_or_config(server_id_or_url))
inside an async function; replace that with using await directly (server = await
client.get_app_or_config(server_id_or_url)) so the coroutine is properly awaited
within the async context and remove the run_async wrapper.
| server = run_async(client.get_app_or_config(server_id_or_url)) | ||
|
|
There was a problem hiding this comment.
Bug: run_async used inside async function
This will fail under a running event loop. Await the coroutine instead.
- server = run_async(client.get_app_or_config(server_id_or_url))
+ server = await client.get_app_or_config(server_id_or_url)📝 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 = run_async(client.get_app_or_config(server_id_or_url)) | |
| server = await client.get_app_or_config(server_id_or_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/list/main.py around lines 29-30,
run_async is being used to execute a coroutine inside an already async function
which will fail when an event loop is running; replace the run_async call by
directly awaiting the coroutine (i.e., assign server = await
client.get_app_or_config(server_id_or_url)), ensure the containing function is
declared async, and remove any now-unused run_async import.
| server = run_async(client.get_app_or_config(server_id_or_url)) | ||
|
|
There was a problem hiding this comment.
Bug: run_async used inside async function
Await instead.
- server = run_async(client.get_app_or_config(server_id_or_url))
+ server = await client.get_app_or_config(server_id_or_url)📝 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 = run_async(client.get_app_or_config(server_id_or_url)) | |
| server = await client.get_app_or_config(server_id_or_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/resume/main.py around lines 31-32,
the code calls run_async(...) from inside an async function which is incorrect;
replace the run_async call with an await of the coroutine (server = await
client.get_app_or_config(server_id_or_url)), and remove or stop using the
run_async helper/import if it becomes unused.
| server = run_async(client.get_app_or_config(server_id_or_url)) | ||
|
|
There was a problem hiding this comment.
Bug: run_async used inside async function
Await instead.
- server = run_async(client.get_app_or_config(server_id_or_url))
+ server = await client.get_app_or_config(server_id_or_url)📝 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 = run_async(client.get_app_or_config(server_id_or_url)) | |
| server = await client.get_app_or_config(server_id_or_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/runs/main.py around lines 29-30,
the code calls run_async(client.get_app_or_config(server_id_or_url)) inside an
async function; replace this with awaiting the coroutine directly: change the
call to await client.get_app_or_config(server_id_or_url) and assign that result
to server, removing the run_async wrapper so the coroutine is awaited properly.
TL;DR
Unified server resolution logic across CLI commands by implementing a centralized
get_app_or_configmethod.What changed?
resolve_serverandparse_app_identifierutility functionsclient.get_app_or_configtail_logscommand to accept server URLs as inputHow to test?
Test the logger tail command with different identifier types:
Test server commands with different identifier types:
Test workflow commands with server URLs:
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores