Fix: Fix Up Workflow Commands #466
Conversation
WalkthroughRefactors example formatting; migrates CLI workflow commands from MCPApp/SSE to API-key + mcp_connection_session; adds shared workflow-printing utilities; extends MCP client session with status/cancel/resume/deserialization APIs; normalizes messages and error handling; adapts JSON/YAML/text output paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI Command
participant Auth as load_api_key_credentials
participant Sess as mcp_connection_session
participant Srv as MCP Server
participant UI as print_* utilities
U->>CLI: invoke workflows command (list/describe/runs/cancel/resume)
CLI->>Auth: load API key
alt no API key
CLI-->>U: CLIError -> "please login"
else API key present
CLI->>Sess: open(session_url, api_key)
Sess->>Srv: authenticate/request
Srv-->>Sess: response (Workflow/WorkflowRun data)
Sess-->>CLI: typed models (Workflow/WorkflowRun)
CLI->>UI: print_workflows / print_workflow_runs / print_workflow_status
UI-->>U: formatted output (text/JSON/YAML)
end
sequenceDiagram
autonumber
participant CLI as resume/cancel command
participant Sess as MCPClientSession
participant Srv as MCP Server
participant UI as console/print_error
CLI->>Sess: resume_workflow(run_id, signal_name, payload)
Sess->>Srv: signal/cancel request
alt success
Srv-->>Sess: success
Sess-->>CLI: True
CLI->>UI: print success banner & Run ID
else failure
Srv-->>Sess: error
Sess-->>CLI: False / raises
CLI->>UI: print_error (non-fatal) or raise CLIError (outer)
end
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 |
There was a problem hiding this comment.
Wow, incredible work @rholinshead. Looks like a lot of work 🏃🏼
![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 (4)
examples/workflows/workflow_orchestrator_worker/main.py (1)
128-132: Fix stray quote in triple-quoted instruction stringThe current value starts with
""""which introduces an unintended leading"in the string. Use a proper triple-quoted literal.Apply this diff:
- proofreader = Agent( - name="proofreader", - instruction=""""Review the short story for grammar, spelling, and punctuation errors. - Identify any awkward phrasing or structural issues that could improve clarity. - Provide detailed feedback on corrections.""", - server_names=["fetch"], - ) + proofreader = Agent( + name="proofreader", + instruction="""Review the short story for grammar, spelling, and punctuation errors. + Identify any awkward phrasing or structural issues that could improve clarity. + Provide detailed feedback on corrections.""", + server_names=["fetch"], + )src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (1)
61-68: Ensure non-zero exit on cancellation failurePrinting and continuing leads to a success exit even when cancel fails. Raise
CLIErrorso the CLI exits with an error code; avoid double-wrapping by lettingCLIErrorpass through.Apply this diff:
- else: - print_error(f"Failed to cancel workflow with run ID {run_id}") - except Exception as e: - print_error(f"Error cancelling workflow with run ID {run_id}: {str(e)}") + else: + raise CLIError(f"Failed to cancel workflow with run ID {run_id}") + except CLIError: + # Bubble up cleanly + raise + except Exception as e: + raise CLIError( + f"Error cancelling workflow with run ID {run_id}: {str(e)}" + ) from e @@ - except Exception as e: - raise CLIError( - f"Error cancelling workflow with run ID {run_id}: {str(e)}" - ) from e + except CLIError: + raise + except Exception as e: + raise CLIError( + f"Error cancelling workflow with run ID {run_id}: {str(e)}" + ) from esrc/mcp_agent/cli/cloud/commands/workflows/resume/main.py (1)
20-25: Payload handling bug: JSON is detected but not parsed before sending; type mismatch with client (expects dict).Parse JSON once and pass parsed object; keep raw text otherwise. Also make async helper accept the parsed type.
-from typing import Optional +from typing import Optional, Any, Union @@ -async def _signal_workflow_async( +async def _signal_workflow_async( server_id_or_url: str, run_id: str, signal_name: str = "resume", - payload: Optional[str] = None, + payload: Optional[Union[dict[str, Any], str]] = None, ) -> None: @@ - success = await mcp_client_session.resume_workflow( - run_id, signal_name, payload - ) + success = await mcp_client_session.resume_workflow( + run_id, signal_name, payload + ) @@ - if payload: - try: - json.loads(payload) - console.print("[dim]Resuming with JSON payload...[/dim]") - except json.JSONDecodeError: - console.print("[dim]Resuming with text payload...[/dim]") + parsed_payload: Optional[Union[dict[str, Any], str]] = None + if payload: + try: + parsed_payload = json.loads(payload) + console.print("[dim]Resuming with JSON payload...[/dim]") + except json.JSONDecodeError: + parsed_payload = payload + console.print("[dim]Resuming with text payload...[/dim]") @@ - run_async( - _signal_workflow_async( - server_id_or_url, run_id, signal_name or "resume", payload - ) - ) + run_async( + _signal_workflow_async( + server_id_or_url, run_id, signal_name or "resume", parsed_payload + ) + )Also applies to: 64-67, 137-148
src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (1)
118-132: Status filter bug: mapping 'failed' → 'error' hides Failed runs.Server statuses include “Failed”. Mapping user input
failedtoerrorprevents matches. Keepfailedasfailed.status_map = { "running": "running", - "failed": "error", - "error": "error", + "failed": "failed", + "error": "error", "timed_out": "timed_out", "timeout": "timed_out", # alias "canceled": "canceled", "cancelled": "canceled", # alias "terminated": "terminated", "completed": "completed", "continued": "continued", "continued_as_new": "continued", }
🧹 Nitpick comments (11)
examples/workflows/workflow_intent_classifier/main.py (1)
78-80: Add a newline between the two result sections for readabilityCurrently the two outputs concatenate without separation.
Apply this diff:
- results += "LLM-based Intent classification results: " + ", ".join( + results += "\nLLM-based Intent classification results: " + ", ".join( r.intent for r in output )src/mcp_agent/cli/cloud/commands/app/workflows/main.py (2)
57-60: Fix grammar in login error message.Add missing "to" and improve clarity.
- "Must be logged in list workflow details. Run 'mcp-agent login', set MCP_API_KEY environment variable or specify --api-key option." + "Must be logged in to list workflow details. Run 'mcp-agent login', set the MCP_API_KEY environment variable, or specify --api-key."
166-175: Harden sort key against unexpected types for start_time.If start_time ever arrives as a string/ISO (seen in some backends), sorting can raise or misorder. Coerce to epoch seconds.
- def get_start_time(run: WorkflowRun): - try: - return ( - run.temporal.start_time - if run.temporal and run.temporal.start_time is not None - else 0 - ) - except AttributeError: - return 0 + def get_start_time(run: WorkflowRun): + try: + ts = run.temporal.start_time if run.temporal else None + if ts is None: + return 0 + if isinstance(ts, (int, float)): + return float(ts) + # Fallback for ISO/string inputs + from datetime import datetime + try: + return datetime.fromisoformat(str(ts).replace("Z", "+00:00")).timestamp() + except Exception: + return 0 + except AttributeError: + return 0src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (1)
111-121: Expose --signal-name explicitly for better CLI help text.Typer will create an option implicitly, but adding typer.Option gives clear help.
- signal_name: Optional[str] = "resume", + signal_name: Optional[str] = typer.Option( + "resume", "--signal-name", help="Signal to send (e.g., resume, suspend, provide_human_input)" + ),src/mcp_agent/cli/mcp_app/mcp_client.py (3)
12-13: Avoid mutable defaults in Pydantic models.Use Field(default_factory=...) for list/dict fields.
-from pydantic import AnyUrl, BaseModel +from pydantic import AnyUrl, BaseModel, Field @@ - capabilities: Optional[list[str]] = [] + capabilities: Optional[list[str]] = Field(default_factory=list) @@ - tool_endpoints: Optional[list[str]] = [] + tool_endpoints: Optional[list[str]] = Field(default_factory=list) @@ - run_parameters: Optional[dict[str, Any]] = {} + run_parameters: Optional[dict[str, Any]] = Field(default_factory=dict)Also applies to: 32-39
343-347: Don’t send Authorization header with None value.Some clients misbehave with None headers.
- kwargs = { - "url": str(self.server_url), - "headers": { - "Authorization": (f"Bearer {self._api_key}" if self._api_key else None), - }, - } + headers = {} + if self._api_key: + headers["Authorization"] = f"Bearer {self._api_key}" + kwargs = { + "url": str(self.server_url), + "headers": headers, + }
376-377: Remove unconditional console noise during session init.Prefer debug logging to avoid cluttering CLI output.
- console.print("Initializing MCPClientSession") + # console.print("Initializing MCPClientSession") # debug onlysrc/mcp_agent/cli/cloud/commands/workflows/utils.py (2)
41-51: Avoid mutating the original schema; deep-copy before edits.Shallow copy still mutates nested dicts.
+import copy @@ - schema = schema.copy() + schema = copy.deepcopy(schema)
145-161: Consider consistent timezone handling for epoch timestamps.datetime.fromtimestamp() uses local time; using UTC avoids environment-dependent output.
- dt = datetime.fromtimestamp(started_at) + dt = datetime.utcfromtimestamp(started_at)src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (1)
16-20: Use sharedvalidate_output_formatfor consistency with other commands.The runs command already uses
validate_output_format; mirror that here.from ...utils import ( setup_authenticated_client, handle_server_api_errors, - resolve_server, + resolve_server, + validate_output_format, ) @@ - if format not in ["text", "json", "yaml"]: - console.print("[red]Error: --format must be 'text', 'json', or 'yaml'[/red]") - raise typer.Exit(6) + validate_output_format(format)Also applies to: 89-93
src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (1)
162-171: Prefer JSON-safe dumps for model items.For consistency with describe, consider using JSON mode for YAML as well.
def _print_workflows_json(workflows: list[WorkflowRun]): """Print workflows in JSON format.""" - workflows_data = [workflow.model_dump() for workflow in workflows] + workflows_data = [workflow.model_dump(mode="json") for workflow in workflows] print(json.dumps({"workflow_runs": workflows_data}, indent=2, default=str)) @@ def _print_workflows_yaml(workflows: list[WorkflowRun]): """Print workflows in YAML format.""" - workflows_data = [workflow.model_dump() for workflow in workflows] + workflows_data = [workflow.model_dump(mode="json") for workflow in workflows] print(yaml.dump({"workflow_runs": workflows_data}, default_flow_style=False))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
examples/workflows/workflow_intent_classifier/main.py(3 hunks)examples/workflows/workflow_orchestrator_worker/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/app/status/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/app/workflows/main.py(4 hunks)src/mcp_agent/cli/cloud/commands/logger/tail/main.py(1 hunks)src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py(3 hunks)src/mcp_agent/cli/cloud/commands/workflows/describe/main.py(3 hunks)src/mcp_agent/cli/cloud/commands/workflows/list/main.py(2 hunks)src/mcp_agent/cli/cloud/commands/workflows/resume/main.py(6 hunks)src/mcp_agent/cli/cloud/commands/workflows/runs/main.py(6 hunks)src/mcp_agent/cli/cloud/commands/workflows/utils.py(1 hunks)src/mcp_agent/cli/mcp_app/mcp_client.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/mcp_agent/cli/cloud/commands/workflows/utils.py (2)
src/mcp_agent/cli/mcp_app/mcp_client.py (2)
Workflow(23-38)WorkflowRun(110-138)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (6)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/cloud/commands/workflows/utils.py (1)
format_workflow_status(15-37)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/mcp_client.py (3)
WorkflowRun(110-138)mcp_connection_session(382-409)get_workflow_status(223-256)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)
src/mcp_agent/cli/cloud/commands/workflows/list/main.py (6)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/cloud/commands/workflows/utils.py (1)
print_workflows(54-112)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/mcp_client.py (1)
mcp_connection_session(382-409)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)
src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (5)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/mcp_client.py (2)
mcp_connection_session(382-409)cancel_workflow(258-283)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)
src/mcp_agent/cli/cloud/commands/app/workflows/main.py (2)
src/mcp_agent/cli/cloud/commands/workflows/utils.py (2)
print_workflows(54-112)print_workflow_runs(115-168)src/mcp_agent/cli/mcp_app/mcp_client.py (2)
MCPClientSession(147-317)list_workflows(150-177)
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (3)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/mcp_app/mcp_client.py (2)
mcp_connection_session(382-409)resume_workflow(285-317)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)
src/mcp_agent/cli/mcp_app/mcp_client.py (3)
src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (1)
cancel_workflow(72-92)src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (1)
resume_workflow(112-148)
examples/workflows/workflow_orchestrator_worker/main.py (2)
src/mcp_agent/app.py (1)
tool(715-767)examples/basic/mcp_basic_agent/main.py (1)
example_usage(51-115)
src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (7)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/cloud/commands/workflows/utils.py (1)
print_workflow_runs(115-168)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/mcp_client.py (3)
WorkflowRun(110-138)mcp_connection_session(382-409)list_workflow_runs(179-201)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)src/mcp_agent/cli/cloud/commands/utils.py (1)
resolve_server(51-70)
🔇 Additional comments (12)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (1)
407-411: No-op formatting change is fineAdding a trailing comma in this multi-arg call is harmless and consistent with common formatters. LGTM.
src/mcp_agent/cli/cloud/commands/app/status/main.py (1)
165-168: Wording tweak improves clarityRephrased error message better reflects the action performed. No functional impact.
examples/workflows/workflow_orchestrator_worker/main.py (1)
92-101: Style cleanups look goodSignature spacing, docstring quotes, and initializing result improve readability without behavior changes.
examples/workflows/workflow_intent_classifier/main.py (1)
15-24: Signature/docstring formatting LGTMSpacing and docstring delimiter changes align with project style.
src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py (1)
45-53: Migration to session-based MCP client is solidUsing
mcp_connection_sessionwith a spinner improves UX and reduces previous TaskGroup issues. Nice.src/mcp_agent/cli/cloud/commands/app/workflows/main.py (1)
8-11: Good consolidation to shared renderers.Importing and delegating to print_workflows/print_workflow_runs reduces duplication and aligns outputs across commands.
Also applies to: 144-145, 182-183
src/mcp_agent/cli/cloud/commands/workflows/list/main.py (1)
58-72: JSON/YAML paths look solid.model_dump() with default=str and yaml.dump is appropriate for these models.
src/mcp_agent/cli/cloud/commands/workflows/utils.py (1)
15-38: Status formatter is clear and robust.Good normalization and emoji mapping.
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (2)
57-60: Confirm desired exit code behavior on retrieval failures.Inner exceptions are printed but not raised; the command exits 0 even if describe fails. Confirm this is intentional; otherwise re-raise a
CLIErrorafterprint_error(...).Do you want a follow-up patch to return a non-zero exit on failures?
Also applies to: 62-65
48-56: Resolved —mcp_connection_sessionis an async context manager.
Confirmed @asynccontextmanager decorator and async def at src/mcp_agent/cli/mcp_app/mcp_client.py (around lines 381–385).src/mcp_agent/cli/cloud/commands/workflows/runs/main.py (2)
76-80: Confirm exit code on list failures.Errors are printed but not surfaced; command returns 0 even on failure. Confirm intent; otherwise raise a
CLIErrorafter printing.
45-49: Credential check LGTM.Clear error when not logged in; consistent with other commands.
| from rich.panel import Panel | ||
| from rich.prompt import Prompt |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove duplicate “no runs” panel; defer to shared utility for consistent output.
This block reintroduces bespoke rendering and diverges from print_workflow_runs() behavior noted elsewhere in the PR.
-from rich.panel import Panel
@@
- if not res.workflow_runs:
- console.print(
- Panel(
- "[yellow]No workflow runs found[/yellow]",
- title="Workflow Runs",
- border_style="blue",
- )
- )
- return
+ if not res.workflow_runs:
+ print_workflow_runs([])
+ returnAlso applies to: 156-165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- search: occurrences of print_workflow_runs ----"
rg -n --hidden -S "print_workflow_runs" -g '!*node_modules/*' || true
echo
echo "---- search: definition of print_workflow_runs ----"
rg -n --hidden -S "def print_workflow_runs" -g '!*node_modules/*' || true
echo
echo "---- search: 'No workflow runs found' literal ----"
rg -n --hidden -S "No workflow runs found" -g '!*node_modules/*' || true
echo
echo "---- search: Panel( usage ----"
rg -n --hidden -S "Panel\\(" -g '!*node_modules/*' || true
echo
FILE="src/mcp_agent/cli/cloud/commands/app/workflows/main.py"
if [ -f "$FILE" ]; then
echo "---- head of $FILE ----"
sed -n '1,120p' "$FILE" || true
echo
echo "---- lines ~140-180 of $FILE ----"
sed -n '140,180p' "$FILE" || true
else
echo "File not found: $FILE"
fiLength of output: 6215
Replace local "No workflow runs" Panel with print_workflow_runs([]) and remove Panel import.
Use the shared print_workflow_runs for consistent output; drop the Panel import in this file.
-from rich.panel import Panel
@@
- if not res.workflow_runs:
- console.print(
- Panel(
- "[yellow]No workflow runs found[/yellow]",
- title="Workflow Runs",
- border_style="blue",
- )
- )
- return
+ if not res.workflow_runs:
+ print_workflow_runs([])
+ return[src/mcp_agent/cli/cloud/commands/app/workflows/main.py — import lines and print_runs_list block]
📝 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.
| from rich.panel import Panel | |
| from rich.prompt import Prompt | |
| from rich.prompt import Prompt | |
| if not res.workflow_runs: | |
| print_workflow_runs([]) | |
| return |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/app/workflows/main.py around lines 4-5,
remove the unused Panel import and replace the local Panel("No workflow runs",
...) usage with a call to the shared print_workflow_runs([]) helper for
consistent output; if print_workflow_runs is not already imported, add the
appropriate import from the shared printing module, then delete the Panel import
line.
| effective_api_key = load_api_key_credentials() | ||
|
|
||
| try: | ||
| async with app.run() as workflow_app: | ||
| context = workflow_app.context | ||
|
|
||
| sse_url = ( | ||
| f"{server_url.rstrip('/')}/sse" | ||
| if not server_url.endswith("/sse") | ||
| else server_url | ||
| ) | ||
| context.server_registry.registry["workflow_server"] = MCPServerSettings( | ||
| name="workflow_server", | ||
| description=f"Deployed MCP server {server_url}", | ||
| url=sse_url, | ||
| transport="sse", | ||
| ) | ||
| if not effective_api_key: | ||
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also honor env-configured API keys to avoid false “not logged in” errors
This only reads the stored credentials file. Align with other commands by also checking settings.API_KEY (env-backed).
Apply this diff in-place:
- effective_api_key = load_api_key_credentials()
+ # Prefer env/configured API key, fall back to stored credentials
+ from mcp_agent.cli.config import settings
+ effective_api_key = settings.API_KEY or load_api_key_credentials()And ensure the import exists at the top (outside this range):
+from mcp_agent.cli.config import settings📝 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.
| effective_api_key = load_api_key_credentials() | |
| try: | |
| async with app.run() as workflow_app: | |
| context = workflow_app.context | |
| sse_url = ( | |
| f"{server_url.rstrip('/')}/sse" | |
| if not server_url.endswith("/sse") | |
| else server_url | |
| ) | |
| context.server_registry.registry["workflow_server"] = MCPServerSettings( | |
| name="workflow_server", | |
| description=f"Deployed MCP server {server_url}", | |
| url=sse_url, | |
| transport="sse", | |
| ) | |
| if not effective_api_key: | |
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") | |
| # Prefer env/configured API key, fall back to stored credentials | |
| from mcp_agent.cli.config import settings | |
| effective_api_key = settings.API_KEY or load_api_key_credentials() | |
| if not effective_api_key: | |
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") |
| effective_api_key = load_api_key_credentials() | |
| try: | |
| async with app.run() as workflow_app: | |
| context = workflow_app.context | |
| sse_url = ( | |
| f"{server_url.rstrip('/')}/sse" | |
| if not server_url.endswith("/sse") | |
| else server_url | |
| ) | |
| context.server_registry.registry["workflow_server"] = MCPServerSettings( | |
| name="workflow_server", | |
| description=f"Deployed MCP server {server_url}", | |
| url=sse_url, | |
| transport="sse", | |
| ) | |
| if not effective_api_key: | |
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") | |
| from mcp_agent.cli.config import settings |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/cancel/main.py around lines 39 to
43, the code only checks stored credentials and raises a CLIError when none are
found; update it to also honor env-configured keys by using settings.API_KEY as
a fallback (i.e., if load_api_key_credentials() returns falsy, use
settings.API_KEY before raising), and ensure settings is imported at the top of
the file.
| if format == "json": | ||
| print(json.dumps(workflow_status.model_dump(), indent=2)) | ||
| elif format == "yaml": | ||
| print(yaml.dump(workflow_status.model_dump(), default_flow_style=False)) | ||
| else: # text format |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make JSON/YAML serialization robust for Pydantic v2 objects.
model_dump() may include non-JSON-serializable types (e.g., datetimes). Prefer model_dump_json() for JSON and model_dump(mode="json") for YAML.
- if format == "json":
- print(json.dumps(workflow_status.model_dump(), indent=2))
- elif format == "yaml":
- print(yaml.dump(workflow_status.model_dump(), default_flow_style=False))
+ if format == "json":
+ print(workflow_status.model_dump_json(indent=2))
+ elif format == "yaml":
+ print(yaml.dump(workflow_status.model_dump(mode="json"), 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.
| if format == "json": | |
| print(json.dumps(workflow_status.model_dump(), indent=2)) | |
| elif format == "yaml": | |
| print(yaml.dump(workflow_status.model_dump(), default_flow_style=False)) | |
| else: # text format | |
| if format == "json": | |
| print(workflow_status.model_dump_json(indent=2)) | |
| elif format == "yaml": | |
| print(yaml.dump(workflow_status.model_dump(mode="json"), default_flow_style=False)) | |
| else: # text format |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/describe/main.py around lines
99-103, the code calls model_dump() and then json.dumps/yaml.dump which can fail
on Pydantic v2 types; replace the JSON branch to use
workflow_status.model_dump_json(indent=2) and print its string directly, and
replace the YAML branch to call workflow_status.model_dump(mode="json") and pass
that resulting plain-serializable structure into yaml.dump(...,
default_flow_style=False) so datetimes and other Pydantic v2 types are converted
to JSON-safe representations before serialization.
| from datetime import datetime | ||
|
|
||
| created_dt = datetime.fromtimestamp(created_at) | ||
| created_at = created_dt.strftime("%Y-%m-%d %H:%M:%S") | ||
| except (ValueError, TypeError): | ||
| created_at = str(created_at) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix NameError on datetime and align timestamp parsing with runs list logic.
datetime is imported only when created_at is not None, but later used for result start_time/end_time regardless, which can raise NameError. Also, start_time/end_time may be ISO strings—parse like print_workflow_runs.
Apply within this block:
- try:
- from datetime import datetime
-
- created_dt = datetime.fromtimestamp(created_at)
+ try:
+ created_dt = (
+ datetime.fromtimestamp(created_at)
+ if isinstance(created_at, (int, float))
+ else datetime.fromisoformat(str(created_at).replace("Z", "+00:00"))
+ )
created_at = created_dt.strftime("%Y-%m-%d %H:%M:%S")- if start_time:
- start_dt = datetime.fromtimestamp(start_time).strftime(
- "%Y-%m-%d %H:%M:%S"
- )
- console.print(f" Started: {start_dt}")
+ if start_time:
+ try:
+ dt = (
+ datetime.fromtimestamp(start_time)
+ if isinstance(start_time, (int, float))
+ else datetime.fromisoformat(str(start_time).replace("Z", "+00:00"))
+ )
+ console.print(f" Started: {dt.strftime('%Y-%m-%d %H:%M:%S')}")
+ except (ValueError, TypeError):
+ console.print(f" Started: {start_time}")
- if end_time:
- end_dt = datetime.fromtimestamp(end_time).strftime("%Y-%m-%d %H:%M:%S")
- console.print(f" Ended: {end_dt}")
+ if end_time:
+ try:
+ dt = (
+ datetime.fromtimestamp(end_time)
+ if isinstance(end_time, (int, float))
+ else datetime.fromisoformat(str(end_time).replace("Z", "+00:00"))
+ )
+ console.print(f" Ended: {dt.strftime('%Y-%m-%d %H:%M:%S')}")
+ except (ValueError, TypeError):
+ console.print(f" Ended: {end_time}")And add a top-level import (outside the selected lines):
from datetime import datetimeAlso applies to: 154-163
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/describe/main.py around lines
121-126 (and similarly update lines 154-163), fix a potential NameError and
normalize timestamp parsing: add a top-level import "from datetime import
datetime"; replace the local conditional import logic so datetime is always
available, and parse created_at/start_time/end_time the same way
print_workflow_runs does — i.e., if the value is a numeric timestamp convert
with datetime.fromtimestamp(...) and format with strftime, else if it is an ISO
string parse it to a datetime (or convert to string fallback) so you always
produce consistent formatted strings and avoid referencing datetime when not
imported. Ensure both blocks (121-126 and 154-163) use the same parsing
helper/logic.
| effective_api_key = load_api_key_credentials() | ||
|
|
||
| try: | ||
| async with app.run() as workflow_app: | ||
| context = workflow_app.context | ||
|
|
||
| sse_url = ( | ||
| f"{server_url}/sse" if not server_url.endswith("/sse") else server_url | ||
| ) | ||
| context.server_registry.registry["workflow_server"] = MCPServerSettings( | ||
| name="workflow_server", | ||
| description=f"Deployed MCP server {server_url}", | ||
| url=sse_url, | ||
| transport="sse", | ||
| ) | ||
| if not effective_api_key: | ||
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") | ||
|
|
There was a problem hiding this comment.
Honor MCP_API_KEY env var as fallback; align error text with other commands.
Currently only credential file is read, causing false “Must be logged in” when MCP_API_KEY is set.
-from mcp_agent.cli.core.utils import run_async
+from mcp_agent.cli.core.utils import run_async
+from mcp_agent.cli.config import settings
@@
- effective_api_key = load_api_key_credentials()
+ effective_api_key = settings.API_KEY or load_api_key_credentials()
@@
- if not effective_api_key:
- raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.")
+ if not effective_api_key:
+ raise CLIError("Must be logged in to access server. Run 'mcp-agent login' or set MCP_API_KEY.")📝 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.
| effective_api_key = load_api_key_credentials() | |
| try: | |
| async with app.run() as workflow_app: | |
| context = workflow_app.context | |
| sse_url = ( | |
| f"{server_url}/sse" if not server_url.endswith("/sse") else server_url | |
| ) | |
| context.server_registry.registry["workflow_server"] = MCPServerSettings( | |
| name="workflow_server", | |
| description=f"Deployed MCP server {server_url}", | |
| url=sse_url, | |
| transport="sse", | |
| ) | |
| if not effective_api_key: | |
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login'.") | |
| from mcp_agent.cli.core.utils import run_async | |
| from mcp_agent.cli.config import settings | |
| effective_api_key = settings.API_KEY or load_api_key_credentials() | |
| if not effective_api_key: | |
| raise CLIError("Must be logged in to access server. Run 'mcp-agent login' or set MCP_API_KEY.") |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/list/main.py around lines 41 to
45, the code only reads the credential file and raises "Must be logged in…" even
when MCP_API_KEY is set; update logic to fall back to
os.environ.get("MCP_API_KEY") (or equivalent env read) when
load_api_key_credentials() returns falsy and use that value as the
effective_api_key; also adjust the CLIError text to match other commands
(mention both login and the MCP_API_KEY env var) so the message is consistent
across commands.
| payload: Optional[dict[str, Any]] = None, | ||
| ) -> bool: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden payload type to support text payloads.
Matches CLI behavior after fix and avoids misleading typing.
- payload: Optional[dict[str, Any]] = None,
+ payload: Optional[Union[dict[str, Any], str]] = None,📝 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.
| payload: Optional[dict[str, Any]] = None, | |
| ) -> bool: | |
| payload: Optional[Union[dict[str, Any], str]] = None, | |
| ) -> bool: |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/mcp_app/mcp_client.py around lines 289-290, the payload
parameter is currently typed as Optional[dict[str, Any]] which excludes plain
text payloads; change the type annotation to accept either a dict or a string
(e.g., Optional[Union[dict[str, Any], str]] or Optional[dict[str, Any] | str])
and add the necessary import from typing (Union) or use PEP 604 union syntax,
making corresponding small adjustments to any static type checks or docstrings
that reference the payload type.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (2)
20-25: Propagate operation outcome to callers (return bool instead of swallowing failures).
_signal_workflow_asyncalways returnsNone. Callers cannot surface non-zero exit codes. Make it returnboolto allow the CLI to exit(1) on failure.-async def _signal_workflow_async( +async def _signal_workflow_async( server_id_or_url: str, run_id: str, signal_name: str = "resume", payload: Optional[Dict[str, Any]] = None, -) -> None: +) -> bool:
68-103: Return non-zero on failure and fix user-facing error grammar.Failures only log via
print_errorand then exit 0. Also, messages like “resumeing” read poorly. Return booleans and use the already computedaction_presentstring for consistent wording.if success: action_past = ( "resumed" if signal_name == "resume" else "suspended" if signal_name == "suspend" else f"signaled ({signal_name})" ) action_color = ( "green" if signal_name == "resume" else "yellow" if signal_name == "suspend" else "blue" ) action_icon = ( "✓" if signal_name == "resume" else "⏸" if signal_name == "suspend" else "📡" ) console.print() console.print( f"[{action_color}]{action_icon} Successfully {action_past} workflow[/{action_color}]" ) console.print(f" Run ID: [cyan]{run_id}[/cyan]") + return True else: - print_error( - f"Failed to {signal_name} workflow with run ID {run_id}" - ) + print_error(f"Failed {action_present.lower()} workflow with run ID {run_id}") + return False except Exception as e: - # Don't raise or it will be a generic unhandled error in TaskGroup - print_error( - f"Error {signal_name}ing workflow with run ID {run_id}: {str(e)}" - ) + # Log and signal failure to the caller; CLI will emit a non-zero exit code. + print_error(f"Error {action_present.lower()} workflow with run ID {run_id}: {str(e)}") + return False
♻️ Duplicate comments (3)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (3)
4-4: Good fix: import datetime at module scope.This removes the prior NameError risk when formatting timestamps elsewhere in the file.
100-103: Use Pydantic v2-safe serialization for JSON/YAML.Avoid json.dumps/yaml.dump on model_dump(); use model_dump_json()/mode="json" to serialize datetimes and other types safely.
- if format == "json": - print(json.dumps(workflow_status.model_dump(), indent=2)) - elif format == "yaml": - print(yaml.dump(workflow_status.model_dump(), default_flow_style=False)) + if format == "json": + print(workflow_status.model_dump_json(indent=2)) + elif format == "yaml": + print( + yaml.safe_dump( + workflow_status.model_dump(mode="json"), + default_flow_style=False, + ) + )
114-127: Robust timestamp parsing needed (epoch vs ISO); current code can crash.fromtimestamp() on ISO strings raises TypeError; created_at has a fallback but start/end do not. Parse both numeric epoch and ISO (with ‘Z’) consistently.
Created time block:
- if created_at is not None: - try: - created_dt = datetime.fromtimestamp(created_at) - created_at = created_dt.strftime("%Y-%m-%d %H:%M:%S") - except (ValueError, TypeError): - created_at = str(created_at) + if created_at is not None: + try: + created_dt = ( + datetime.fromtimestamp(created_at) + if isinstance(created_at, (int, float)) + else datetime.fromisoformat(str(created_at).replace("Z", "+00:00")) + ) + created_at = created_dt.strftime("%Y-%m-%d %H:%M:%S") + except (ValueError, TypeError): + created_at = str(created_at)Result timing block:
- if start_time: - start_dt = datetime.fromtimestamp(start_time).strftime( - "%Y-%m-%d %H:%M:%S" - ) - console.print(f" Started: {start_dt}") + if start_time: + try: + dt = ( + datetime.fromtimestamp(start_time) + if isinstance(start_time, (int, float)) + else datetime.fromisoformat(str(start_time).replace("Z", "+00:00")) + ) + console.print(f" Started: {dt.strftime('%Y-%m-%d %H:%M:%S')}") + except (ValueError, TypeError): + console.print(f" Started: {start_time}") - if end_time: - end_dt = datetime.fromtimestamp(end_time).strftime("%Y-%m-%d %H:%M:%S") - console.print(f" Ended: {end_dt}") + if end_time: + try: + dt = ( + datetime.fromtimestamp(end_time) + if isinstance(end_time, (int, float)) + else datetime.fromisoformat(str(end_time).replace("Z", "+00:00")) + ) + console.print(f" Ended: {dt.strftime('%Y-%m-%d %H:%M:%S')}") + except (ValueError, TypeError): + console.print(f" Ended: {end_time}")Also applies to: 153-163
🧹 Nitpick comments (4)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (1)
97-99: Optional: centralize formatting helpers.Consider moving timestamp formatting and value truncation into shared helpers in workflows/utils.py to keep describe/list outputs consistent.
Happy to extract a small utility (format_ts, truncate_value) and wire both describe and list paths to it.
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (3)
137-141: Validate that--payloadis a JSON object (dict), not just any JSON.Server-side expects an object; numbers/arrays/strings will likely error later. Fail fast with a clear message.
if payload: try: payload = json.loads(payload) except json.JSONDecodeError as e: raise typer.BadParameter(f"Invalid JSON payload: {str(e)}") from e + if not isinstance(payload, dict): + raise typer.BadParameter("Payload must be a JSON object (e.g., {\"key\": \"value\"}).")
171-173: Apply the same payload object validation tosuspend_workflow.try: payload = json.loads(payload) except json.JSONDecodeError as e: raise typer.BadParameter(f"Invalid JSON payload: {str(e)}") from e + if not isinstance(payload, dict): + raise typer.BadParameter("Payload must be a JSON object (e.g., {\"key\": \"value\"}).")
4-4: Minor typing consistency.Prefer
dict[str, Any](orMapping[str, Any]) overDict[str, Any]for modern typing consistency (Python 3.9+).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py(3 hunks)src/mcp_agent/cli/cloud/commands/workflows/resume/main.py(6 hunks)src/mcp_agent/cli/mcp_app/mcp_client.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_agent/cli/mcp_app/mcp_client.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T14:31:48.139Z
Learnt from: rholinshead
PR: lastmile-ai/mcp-agent#414
File: src/mcp_agent/logging/logger.py:18-19
Timestamp: 2025-09-05T14:31:48.139Z
Learning: In the mcp-agent logging module (src/mcp_agent/logging/logger.py), temporalio should be imported lazily with try/except ImportError to avoid making it a hard dependency. Use temporalio.workflow.in_workflow() instead of isinstance checks on internal classes like _WorkflowInstanceImpl.
Applied to files:
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (5)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/exceptions.py (1)
CLIError(4-9)src/mcp_agent/cli/mcp_app/mcp_client.py (2)
mcp_connection_session(382-409)resume_workflow(285-317)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)
src/mcp_agent/cli/cloud/commands/workflows/describe/main.py (6)
src/mcp_agent/cli/auth/main.py (1)
load_api_key_credentials(62-69)src/mcp_agent/cli/cloud/commands/workflows/utils.py (1)
format_workflow_status(15-37)src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)src/mcp_agent/cli/mcp_app/mcp_client.py (3)
WorkflowRun(110-138)mcp_connection_session(382-409)get_workflow_status(223-256)src/mcp_agent/cli/utils/ux.py (1)
print_error(80-91)src/mcp_agent/cli/cloud/commands/utils.py (2)
handle_server_api_errors(73-99)setup_authenticated_client(18-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks / test
🔇 Additional comments (1)
src/mcp_agent/cli/cloud/commands/workflows/resume/main.py (1)
48-52: Confirmedmcp_connection_sessionis an async context manager.It's decorated with @asynccontextmanager and defined at src/mcp_agent/cli/mcp_app/mcp_client.py (lines ~381–382), so using it with
async withis correct.
| try: | ||
| async with mcp_connection_session( | ||
| server_url, effective_api_key | ||
| ) as mcp_client_session: | ||
| try: | ||
| workflow_status = await mcp_client_session.get_workflow_status( | ||
| run_id=run_id | ||
| ) | ||
| print_workflow_status(workflow_status, format) | ||
| except Exception as e: | ||
| print_error( | ||
| f"Error getting workflow status from MCP server at {server_url}: {str(e)}" | ||
| ) | ||
|
|
||
| workflow_status = result.content[0].text if result.content else {} | ||
| if isinstance(workflow_status, str): | ||
| workflow_status = json.loads(workflow_status) | ||
|
|
||
| if not workflow_status: | ||
| raise CLIError(f"Workflow with run ID '{run_id}' not found.") | ||
|
|
||
| if format == "json": | ||
| print(json.dumps(workflow_status, indent=2)) | ||
| elif format == "yaml": | ||
| print(yaml.dump(workflow_status, default_flow_style=False)) | ||
| else: # text format | ||
| print_workflow_status(workflow_status) | ||
|
|
There was a problem hiding this comment.
Don’t swallow exceptions; return non-zero on failure.
The inner try/except prints the error but doesn’t raise, causing a silent success (exit 0) even when status retrieval fails.
Apply:
- try:
- workflow_status = await mcp_client_session.get_workflow_status(
- run_id=run_id
- )
- print_workflow_status(workflow_status, format)
- except Exception as e:
- print_error(
- f"Error getting workflow status from MCP server at {server_url}: {str(e)}"
- )
+ try:
+ workflow_status = await mcp_client_session.get_workflow_status(
+ run_id=run_id
+ )
+ print_workflow_status(workflow_status, format)
+ except Exception as e:
+ message = (
+ f"Error getting workflow status from MCP server at {server_url}: {e}"
+ )
+ print_error(message)
+ raise CLIError(message) from e📝 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.
| try: | |
| async with mcp_connection_session( | |
| server_url, effective_api_key | |
| ) as mcp_client_session: | |
| try: | |
| workflow_status = await mcp_client_session.get_workflow_status( | |
| run_id=run_id | |
| ) | |
| print_workflow_status(workflow_status, format) | |
| except Exception as e: | |
| print_error( | |
| f"Error getting workflow status from MCP server at {server_url}: {str(e)}" | |
| ) | |
| workflow_status = result.content[0].text if result.content else {} | |
| if isinstance(workflow_status, str): | |
| workflow_status = json.loads(workflow_status) | |
| if not workflow_status: | |
| raise CLIError(f"Workflow with run ID '{run_id}' not found.") | |
| if format == "json": | |
| print(json.dumps(workflow_status, indent=2)) | |
| elif format == "yaml": | |
| print(yaml.dump(workflow_status, default_flow_style=False)) | |
| else: # text format | |
| print_workflow_status(workflow_status) | |
| try: | |
| async with mcp_connection_session( | |
| server_url, effective_api_key | |
| ) as mcp_client_session: | |
| try: | |
| workflow_status = await mcp_client_session.get_workflow_status( | |
| run_id=run_id | |
| ) | |
| print_workflow_status(workflow_status, format) | |
| except Exception as e: | |
| message = ( | |
| f"Error getting workflow status from MCP server at {server_url}: {e}" | |
| ) | |
| print_error(message) | |
| raise CLIError(message) from e |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/describe/main.py around lines 49
to 62, the inner try/except catches exceptions from get_workflow_status and only
prints the error, causing the command to exit with success; rework the handler
to propagate failure by either re-raising the exception or calling sys.exit(1)
after printing the error so the process returns non-zero on failure, e.g., after
print_error(...) immediately raise the exception or call sys.exit(1) (ensure sys
is imported if using sys.exit).
| signal_name: Optional[str] = "resume", | ||
| payload: Optional[str] = typer.Option( | ||
| None, "--payload", help="JSON or text payload to pass to resumed workflow" | ||
| None, | ||
| "--payload", | ||
| help="JSON payload to pass to resumed workflow", | ||
| ), |
There was a problem hiding this comment.
Make --signal-name a proper Typer option (currently parsed as a positional arg).
Examples/docs show --signal-name, but the parameter is defined as a positional argument. This breaks CLI UX and will confuse users.
Apply:
- signal_name: Optional[str] = "resume",
+ signal_name: Optional[str] = typer.Option(
+ "resume",
+ "--signal-name",
+ help="Signal to send (e.g., resume, suspend, provide_human_input)",
+ ),
payload: Optional[str] = typer.Option(
None,
"--payload",
help="JSON payload to pass to resumed workflow",
),📝 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.
| signal_name: Optional[str] = "resume", | |
| payload: Optional[str] = typer.Option( | |
| None, "--payload", help="JSON or text payload to pass to resumed workflow" | |
| None, | |
| "--payload", | |
| help="JSON payload to pass to resumed workflow", | |
| ), | |
| signal_name: Optional[str] = typer.Option( | |
| "resume", | |
| "--signal-name", | |
| help="Signal to send (e.g., resume, suspend, provide_human_input)", | |
| ), | |
| payload: Optional[str] = typer.Option( | |
| None, | |
| "--payload", | |
| help="JSON payload to pass to resumed workflow", | |
| ), |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/resume/main.py around lines 117 to
122, the parameter signal_name is currently declared as a positional argument
but should be a flagged option; replace its signature to use typer.Option with
the flag name "--signal-name" (and keep the default "resume") and add an
appropriate help string so it is parsed as an optional CLI flag rather than a
positional parameter.
| run_async( | ||
| _signal_workflow_async( | ||
| server_id_or_url, run_id, signal_name or "resume", payload | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exit with a non-zero status when the operation fails.
Ensure scripts/CI can rely on exit codes.
- run_async(
- _signal_workflow_async(
- server_id_or_url, run_id, signal_name or "resume", payload
- )
- )
+ success = run_async(
+ _signal_workflow_async(server_id_or_url, run_id, signal_name or "resume", payload)
+ )
+ if not success:
+ raise typer.Exit(code=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.
| run_async( | |
| _signal_workflow_async( | |
| server_id_or_url, run_id, signal_name or "resume", payload | |
| ) | |
| ) | |
| success = run_async( | |
| _signal_workflow_async(server_id_or_url, run_id, signal_name or "resume", payload) | |
| ) | |
| if not success: | |
| raise typer.Exit(code=1) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/resume/main.py around lines 143 to
147, the call to run_async(_signal_workflow_async(...)) currently does not cause
a non-zero process exit when the async operation fails; update this block to
wrap the run_async invocation in a try/except that catches exceptions from the
async call, logs or prints the error, and calls sys.exit(1) on failure (ensure
sys is imported), otherwise allow normal exit on success.
| except json.JSONDecodeError as e: | ||
| raise typer.BadParameter(f"Invalid JSON payload: {str(e)}") from e | ||
|
|
||
| run_async(_signal_workflow_async(server_id_or_url, run_id, "suspend", payload)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror non-zero exit behavior for suspend.
- run_async(_signal_workflow_async(server_id_or_url, run_id, "suspend", payload))
+ success = run_async(_signal_workflow_async(server_id_or_url, run_id, "suspend", payload))
+ if not success:
+ raise typer.Exit(code=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.
| run_async(_signal_workflow_async(server_id_or_url, run_id, "suspend", payload)) | |
| success = run_async(_signal_workflow_async(server_id_or_url, run_id, "suspend", payload)) | |
| if not success: | |
| raise typer.Exit(code=1) |
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/workflows/resume/main.py around line 175,
the code calls _signal_workflow_async with the action "suspend" and doesn't
mirror the suspend command's non-zero exit behavior; change the action string to
"resume" and ensure you propagate the async result like the suspend
implementation does — await or capture the return/exit status from
run_async(_signal_workflow_async(...)) and call sys.exit(non_zero_code) when the
operation indicates failure so the CLI returns a non-zero exit on errors.
2 participants
Description
Fix up a number of issues with workflow-related commands:
workflowssub commands were failing withunhandled errors in a TaskGroup (1 sub-exception)error; migrated each to use the mcp client instead of creating an MCP Agent app to connect from and fixed some serialization errorsapp workflowsandworkflows list/runs), drop table rendering for workflows and use text since content was being hiddenTesting
e.g.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor