Make workflow_orchestrator_worker cloud ready #459
Conversation
WalkthroughExposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant App
participant Tool as example_usage (tool)
participant Orchestrator as Orchestrator Workflow
Caller->>App: Invoke tool "example_usage"
App->>Tool: Execute tool function
Tool->>Orchestrator: Run orchestrator workflow steps
Orchestrator-->>Tool: Return workflow output (data)
Tool-->>App: Return string result
App-->>Caller: Deliver string response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/workflows/workflow_orchestrator_worker/main.py (1)
126-131: Syntax error: quadruple quotes in proofreader instruction
instruction=""""Review ..."""starts with four quotes and won’t parse. This prevents module import.- proofreader = Agent( + proofreader = Agent( name="proofreader", - instruction=""""Review the short story for grammar, spelling, and punctuation errors. + 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"], )
🧹 Nitpick comments (3)
examples/workflows/workflow_orchestrator_worker/main.py (3)
93-99: PEP 8/docstring nits and redundant initTiny polish: add a space before the return type, prefer triple-double-quoted docstrings with a leading capital, and drop the redundant
result=""initialization.-async def example_usage()->str: - ''' - this example function/tool call will use an orchestrator workflow - to dynamically plan and execute across a number of agents to grade - a short story. - ''' - result="" +async def example_usage() -> str: + """ + Example function/tool that uses an orchestrator workflow to dynamically plan + and execute across multiple agents to grade a short story. + """ + result = ""
106-108: Idempotent and portable filesystem root for cloud runsAppending
os.getcwd()on every invocation can duplicate roots over time and may not point to the intended directory in hosted environments. Make the root configurable and add only once; default to this file’s directory if env not set.-# Add the current directory to the filesystem server's args -context.config.mcp.servers["filesystem"].args.extend([os.getcwd()]) +# Add a single filesystem root, configurable for cloud deployments +fs_root = os.environ.get("MCP_FS_ROOT") +if not fs_root: + # Default to the directory containing this example for portability + from pathlib import Path + fs_root = str(Path(__file__).resolve().parent) +fs_args = context.config.mcp.servers["filesystem"].args +if fs_root not in fs_args: + fs_args.append(fs_root)Note: if you adopt this, add
from pathlib import Pathat the top-level imports.
172-174: Avoid hardcoding the model; prefer config/env for cloud portabilityHardcoding
"gpt-4o"can break in environments with different defaults/quotas. Prefer the app/config model mapping or an env override.- result = await orchestrator.generate_str( - message=task, request_params=RequestParams(model="gpt-4o") - ) + model = os.environ.get("MCP_MODEL") or context.config.models.default # falls back to app config + result = await orchestrator.generate_str( + message=task, request_params=RequestParams(model=model) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/workflows/workflow_orchestrator_worker/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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)
🔇 Additional comments (1)
examples/workflows/workflow_orchestrator_worker/main.py (1)
186-186: Returning the workflow output is goodExposing the value via the tool return makes this cloud-friendly and scriptable.
| @app.tool | ||
| async def example_usage()->str: | ||
| ''' | ||
| this example function/tool call will use an orchestrator workflow | ||
| to dynamically plan and execute across a number of agents to grade | ||
| a short story. | ||
| ''' | ||
| result="" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm async support for @app.tool and avoid nested app.run if unnecessary
The decorator is documented as declaring a synchronous MCP tool; using it on an async def that itself calls async with app.run() risks double-running the app context or returning an un-awaited coroutine, depending on adapter behavior. Please verify that MCPApp.tool fully supports async functions and that nesting app.run() inside a tool is intended.
Run this to audit existing usage patterns and catch potential double-run issues:
If async is not supported, either:
- Convert to a sync tool and delegate to an internal async helper, or
- Keep the function async but register it as a workflow/tool mode that awaits it without wrapping
app.run()again.
🏁 Script executed:
#!/bin/bash
# 1) See how @app.tool is used elsewhere (sync vs async)
rg -nP -C2 '@app\.tool\b' --glob '!**/site-packages/**'
# 2) Find functions that call app.run() inside a decorated tool
rg -nP -C3 '@app\.tool\b.*\n\s*(async\s+def|def)\s+[a-zA-Z_]\w*\(.*\):\n(?s).*?app\.run\('
# 3) Inspect the tool adapter to confirm behavior
rg -nP -C5 'def\s+tool\(' src/mcp_agent/app.pyLength of output: 511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Top-level files:"
ls -la | sed -n '1,120p' || true
echo
echo "1) Search for @app.tool usages (with context)"
rg -n --hidden -S -C3 '@app\.tool\b' || true
echo
echo "2) Search for app.run( usages (with context)"
rg -n --hidden -S -C3 'app\.run\(' || true
echo
echo "3) Print src/mcp_agent/app.py (first 400 lines) to inspect `tool` implementation"
if [ -f src/mcp_agent/app.py ]; then
sed -n '1,400p' src/mcp_agent/app.py || true
else
echo "src/mcp_agent/app.py not found"
fiLength of output: 95284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/mcp_agent/app.py (lines 700-820) ----"
sed -n '700,820p' src/mcp_agent/app.py || echo "src/mcp_agent/app.py: EOF or not found"
echo
echo "---- src/mcp_agent/server/app_server.py (lines 940-1000) ----"
sed -n '940,1000p' src/mcp_agent/server/app_server.py || echo "src/mcp_agent/server/app_server.py: EOF or not found"
echo
echo "---- src/mcp_agent/server/tool_adapter.py (full file) ----"
sed -n '1,400p' src/mcp_agent/server/tool_adapter.py || echo "src/mcp_agent/server/tool_adapter.py: EOF or not found"Length of output: 11872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- src/mcp_agent/app.py (lines 360-520) ----"
sed -n '360,520p' src/mcp_agent/app.py || echo "src/mcp_agent/app.py: EOF or not found"
echo
echo "---- src/mcp_agent/server/app_server.py (lines 1000-1400) ----"
sed -n '1000,1400p' src/mcp_agent/server/app_server.py || echo "src/mcp_agent/server/app_server.py: EOF or not found"Length of output: 22181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for _resolve_workflows_and_context and _get_attached_app definitions..."
rg -n --hidden -S "_resolve_workflows_and_context" || true
rg -n --hidden -S "_get_attached_app" || true
echo
echo "If found, print surrounding lines from src/mcp_agent/server/app_server.py"
rg -n --hidden -S "_resolve_workflows_and_context|_get_attached_app" src/mcp_agent/server/app_server.py -n -C5 || true
echo
echo "Print src/mcp_agent/server/app_server.py around likely helper definitions (lines 1-420)"
sed -n '1,420p' src/mcp_agent/server/app_server.py || trueLength of output: 23098
Remove nested async with app.run() from @app.tool functions — app.tool supports async but app.run() will tear down a server-managed app context
@app.tool is intended to wrap an async function into an auto-generated Workflow (docs/examples and src/mcp_agent/app.py show async @app.tool usage). However MCPApp.run always calls cleanup() on exit (src/mcp_agent/app.py: run), and create_declared_function_tools in src/mcp_agent/server/app_server.py starts the workflow and supplies the app/context for server-exposed tools—so calling async with app.run() inside a function that will be registered as a tool will likely tear down the shared app/context after the tool returns.
- Action: Remove the nested
async with app.run()in examples/workflows/workflow_orchestrator_worker/main.py (≈lines 92–103); instead use the workflow-provided context (accept an app_ctx/app context or use the context passed by the engine). - If the function is only meant to be run as a standalone script, keep
app.run()but do not expose it as an MCP tool; if you need asynchronous start/status endpoints, use @app.async_tool or a Workflow class.
🤖 Prompt for AI Agents
In examples/workflows/workflow_orchestrator_worker/main.py around lines 92 to
99, remove the nested "async with app.run()" call inside the @app.tool async
function because tearing down a server-managed app context from within a
declared tool will call cleanup() and break the shared app; instead have the
tool use the workflow-provided app/context (accept an app_ctx or app parameter
or use the provided engine/context passed into the workflow) to access
resources, or convert this function to a standalone script that calls app.run()
but is not exposed as an MCP tool; update the function signature and internal
usage to rely on the injected context rather than starting/stopping the app.
2 participants
Local run:
Cloud run:
Summary by CodeRabbit
New Features
Improvements
Documentation