Fix: Fix app workflows list-runs request & presentation #458
Conversation
WalkthroughAdds token summary display to basic agent example; normalizes and enriches workflow run/result structures and parsing in the MCP client; refactors CLI workflow runs output from table to per-run blocks with status icons and duration; standardizes formatting in examples and logger tail; updates example function signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (app workflows)
participant Client as MCPClient
participant API as Cloud API
User->>CLI: list workflow runs
CLI->>Client: list_workflow_runs(server)
Client->>API: GET /workflows/runs
API-->>Client: JSON runs[]
loop For each run
alt result is string
Client->>Client: ast.literal_eval(result)
alt ast fails
Client->>Client: json.loads(result)
alt json fails
Client->>Client: raise ValueError("Invalid workflow run result data")
end
end
else result is object
Client->>Client: use as-is
end
Client->>Client: attach kind, metadata?, execution_time?
end
Client-->>CLI: runs (normalized)
loop Render each run
CLI->>CLI: Print name/status icon, IDs, times, duration
end
sequenceDiagram
participant Dev as Example (basic agent)
participant App as MCPApp
participant Agent as Agent (optional)
Dev->>App: get_token_summary()
App-->>Dev: totals
alt Agent provided
Dev->>Agent: token usage?
Agent-->>Dev: per-agent usage
end
Dev->>Dev: display_token_summary(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ 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 |
| @@ -1,3 +1,4 @@ | |||
| import ast | |||
There was a problem hiding this comment.
For the ast.literal_eval deserialization (stringified python case) below
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (2)
139-166: Avoid re-creating the authenticated client in _fetch_logs.
You already have client; pass it through to reuse credentials and avoid redundant setup.Apply alongside the signature update below:
- asyncio.run( - _fetch_logs( + asyncio.run( + _fetch_logs( server=server, since=since, grep_pattern=grep, limit=limit, order_by=order_by, asc=asc, desc=desc, format=format, app_identifier=app_identifier, + client=client, ) )And update the helper (outside this hunk):
-async def _fetch_logs( - server: Union[MCPApp, MCPAppConfiguration], +async def _fetch_logs( + server: Union[MCPApp, MCPAppConfiguration], since: Optional[str], grep_pattern: Optional[str], limit: int, order_by: Optional[str], asc: bool, desc: bool, format: str, app_identifier: str, + client, ) -> None: @@ - client = setup_authenticated_client() + # client provided by caller to avoid re-auth/extra setup
268-272: Message clarity nit.
“Server URL not available - server may not be deployed” is fine; consider “Server URL unavailable; the app may not be deployed.” for consistency with other messages.examples/basic/mcp_model_selector/main.py (1)
35-35: Prefer f-strings and join to reduce repeated string concatenations.
Keeps the sample concise and faster under CPython.Example refactor:
- result += "Smartest OpenAI model: " + model.name + result += f"Smartest OpenAI model: {model.name}" @@ - result += "\nMost balanced OpenAI model: " + model.name + result += f"\nMost balanced OpenAI model: {model.name}" @@ - result += "\nFastest and cheapest OpenAI model: " + model.name + result += f"\nFastest and cheapest OpenAI model: {model.name}" @@ - result += "\nSmartest Anthropic model: " + model.name + result += f"\nSmartest Anthropic model: {model.name}" @@ - result += "\nCheapest Anthropic model: " + model.name + result += f"\nCheapest Anthropic model: {model.name}" @@ - result += "\nSelect fastest model between gpt-4o/mini/sonnet/haiku: " + model.name + result += f"\nSelect fastest model between gpt-4o/mini/sonnet/haiku: {model.name}" @@ - result += "\nMost balanced model between gpt-4o/mini/sonnet/haiku: " + model.name + result += f"\nMost balanced model between gpt-4o/mini/sonnet/haiku: {model.name}" @@ - result += "\nBest model with context window >100k tokens: " + model.name + result += f"\nBest model with context window >100k tokens: {model.name}" @@ - result += ( - "\nBest model with 50k-150k context window and tool calling: " + model.name - ) + result += ( + f"\nBest model with 50k-150k context window and tool calling: {model.name}" + ) @@ - result += ( - "\nFastest model with both tool calling and structured outputs: " + model.name - ) + result += ( + f"\nFastest model with both tool calling and structured outputs: {model.name}" + )Also applies to: 48-48, 61-61, 74-74, 87-87, 105-105, 123-123, 143-143, 198-200, 219-221
examples/basic/mcp_basic_agent/main.py (2)
53-56: Docstring grammar nit.
“uses both OpenAI, Anthropic” → “uses both OpenAI and Anthropic.”- The example uses both OpenAI, Anthropic, and simulates a multi-turn conversation. + The example uses both OpenAI and Anthropic, and simulates a multi-turn conversation.
118-156: Token summary helper — nice addition.
Clear, readable breakdown; works with or without agent supplied.Optionally return the formatted string instead of printing (so callers can log or test), and add an optional file-like stream parameter defaulting to sys.stdout.
src/mcp_agent/cli/mcp_app/mcp_client.py (2)
66-68: Result.kind now required — verify server payloads or make it tolerant.
Some servers may not emit a kind; making this required can raise validation errors.Option A (tolerant model):
-class WorkflowRunResult(BaseModel): +class WorkflowRunResult(BaseModel): @@ - kind: str + kind: str | None = NoneOption B (normalize during parsing): see the list_workflow_runs suggestion below to default kind to "text" when absent.
167-175: Workflows parsing assumes dict-of-workflows; support list/singleton too.
Some servers return a list; broaden parsing to avoid failures.- workflow_data = json.loads(item.text) - for value in workflow_data.values(): - workflows.append( - Workflow( - **value, - ) - ) + payload = json.loads(item.text) + if isinstance(payload, dict): + iterable = payload.values() + elif isinstance(payload, list): + iterable = payload + else: + iterable = [payload] + for value in iterable: + workflows.append(Workflow(**value))src/mcp_agent/cli/cloud/commands/app/workflows/main.py (2)
246-247: Minor header formatting nit.
Drop the leading space before “Workflow Runs”.-console.print(f"\n[bold blue] Workflow Runs ({len(sorted_runs)})[/bold blue]") +console.print(f"\n[bold blue]Workflow Runs ({len(sorted_runs)})[/bold blue]")
288-293: Use execution_time if provided; otherwise compute.
Currently you compute duration but only when execution_time exists, and you ignore its value. Make it consistent.- # Show execution time if available - if hasattr(run.temporal, "execution_time") and run.temporal.execution_time: - duration = end - start if (start and end) else None - if duration: - console.print(f" Duration: {duration:.2f}s") + # Show duration + duration = None + if run.temporal: + if getattr(run.temporal, "execution_time", None): + duration = run.temporal.execution_time + elif start and end: + duration = end - start + if duration is not None: + console.print(f" Duration: {duration:.2f}s")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/basic/mcp_basic_agent/main.py(2 hunks)examples/basic/mcp_model_selector/main.py(13 hunks)src/mcp_agent/cli/cloud/commands/app/workflows/main.py(2 hunks)src/mcp_agent/cli/cloud/commands/logger/tail/main.py(4 hunks)src/mcp_agent/cli/mcp_app/mcp_client.py(4 hunks)
🧰 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/mcp_app/mcp_client.py
🧬 Code graph analysis (3)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (1)
src/mcp_agent/cli/cloud/commands/utils.py (2)
setup_authenticated_client(18-32)resolve_server(51-70)
examples/basic/mcp_basic_agent/main.py (3)
src/mcp_agent/app.py (1)
tool(715-767)examples/basic/mcp_model_selector/main.py (1)
example_usage(15-223)examples/workflows/workflow_orchestrator_worker/main.py (1)
example_usage(93-179)
examples/basic/mcp_model_selector/main.py (1)
examples/basic/mcp_basic_agent/main.py (1)
example_usage(51-115)
⏰ 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 (10)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (2)
20-23: Import style change only — LGTM.
No semantic changes.
189-195: No-op change (quote style) — OK.
Functionally identical.examples/basic/mcp_model_selector/main.py (2)
15-16: Function signature/return type normalization — good change.
Consistent with other examples and tool return types.
223-224: Explicit return added — good.
Prevents accidental None returns in the example.examples/basic/mcp_basic_agent/main.py (1)
51-57: Signature/docstring tidy — looks good.
Minor doc tweak suggestion below.src/mcp_agent/cli/mcp_app/mcp_client.py (3)
1-1: Import ast for safe literal parsing — good call.
Keeps deserialization safe vs eval.
72-72: metadata Optional=None — good compatibility change.
Prevents empty-dict defaults and better reflects “may be absent.”
106-108: Temporal.execution_time added — good for durations.
No issues.src/mcp_agent/cli/cloud/commands/app/workflows/main.py (2)
268-275: Status styling/emoji — nice clarity boost.
Clear, accessible at-a-glance status.
276-283: Per-run details — good defaults and visibility.
Printing full Run ID and optional Workflow ID matches the PR goal of copyability.
| if "result" in run_data and isinstance(run_data["result"], str): | ||
| try: | ||
| # Could be stringified python dict instead of valid JSON | ||
| run_data["result"] = ast.literal_eval(run_data["result"]) | ||
| except (ValueError, SyntaxError) as e: | ||
| try: | ||
| run_data["result"] = json.loads(run_data["result"]) | ||
| except json.JSONDecodeError: | ||
| raise ValueError( | ||
| f"Invalid workflow run result data: {e}" | ||
| ) from e |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix deserialization + normalize result shape for Pydantic.
Great fallback to literal_eval→json. Add a final normalization so result is always a dict with kind/value to avoid ValidationError when servers return plain strings.
run_data = json.loads(item.text)
if "result" in run_data and isinstance(run_data["result"], str):
try:
# Could be stringified python dict instead of valid JSON
run_data["result"] = ast.literal_eval(run_data["result"])
except (ValueError, SyntaxError) as e:
try:
run_data["result"] = json.loads(run_data["result"])
except json.JSONDecodeError:
raise ValueError(
f"Invalid workflow run result data: {e}"
) from e
+ # Normalize result for model compatibility
+ if "result" in run_data:
+ res = run_data["result"]
+ if isinstance(res, str):
+ run_data["result"] = {"kind": "text", "value": res}
+ elif isinstance(res, dict):
+ run_data["result"].setdefault("kind", "text")
@@
- except json.JSONDecodeError as e:
- raise ValueError(f"Invalid workflow run data: {e}") from e
+ except json.JSONDecodeError as e:
+ raise ValueError(f"Invalid workflow run data: {e}") from eAlso applies to: 210-210
Description
The deserialization of workflow runs is failing right now because it expects JSON but gets a serialized python structure, so fix to try deserializing python before trying json.
Also, clean up the presentation to be a basic list so that full IDs can be easily copied for use in subsequent commands (like getting run results)
Test
BEFORE (with fix):
NOW (fix + updated presentation):
Summary by CodeRabbit