-
Notifications
You must be signed in to change notification settings - Fork 768
Update OrchestratorWorkflow tool docstring #456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdated examples/temporal/orchestrator.py to make the run_orchestrator app_ctx parameter optional (default None) and adjusted the docstring accordingly. Runtime behavior remains the same by falling back to app.context when app_ctx is not provided. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Orchestrator as run_orchestrator
participant App as app
participant Ctx as AppContext
Caller->>Orchestrator: run_orchestrator(input, app_ctx=None|Ctx)
alt app_ctx provided
Orchestrator->>Ctx: use provided app_ctx
else app_ctx not provided
Orchestrator->>App: access app.context
App-->>Orchestrator: default context
end
Orchestrator-->>Caller: result (str)
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. ✨ 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 |
Args: | ||
input: Task description or instruction text. | ||
app_ctx: Optional application context for the workflow. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring line describing the app_ctx parameter is being removed, but the parameter still exists in the function signature. This creates inconsistent documentation where the function has an app_ctx parameter but no documentation for it. The docstring should be updated to describe the app_ctx parameter with its new default value behavior, not removed entirely.
:param app_ctx: Application context (defaults to None) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/temporal/orchestrator.py (2)
35-37
: Fix docstring return type to match implementationDocstring says “WorkflowResult” but the function returns a
str
fromorchestrator.generate_str(...)
.Apply:
- Returns: - A WorkflowResult containing the processed data + Returns: + str: The generated orchestration output.
39-41
: Guard filesystem args and avoid duplicate cwd entries
- Use explicit
None
check to avoid surprise ifAppContext
ever defines falsy semantics.- Avoid KeyError if “filesystem” server is absent.
- Prevent duplicate cwd entries on repeated calls.
Apply:
- context = app_ctx or app.context - context.config.mcp.servers["filesystem"].args.extend([os.getcwd()]) + context = app_ctx if app_ctx is not None else app.context + fs = context.config.mcp.servers.get("filesystem") + if fs is not None: + cwd = os.getcwd() + if cwd not in fs.args: + fs.args.append(cwd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/temporal/orchestrator.py
(1 hunks)
🔇 Additional comments (2)
examples/temporal/orchestrator.py (2)
28-28
: Makingapp_ctx
optional is fineDefaulting
app_ctx
toNone
aligns with the intent to not require callers to pass internal context.
27-28
: app_ctx is already filtered from public tool schemas — no action required.Tool-registration explicitly drops/filters an app_ctx parameter when generating function signatures/schemas (see src/mcp_agent/server/app_server.py and src/mcp_agent/server/tool_adapter.py); src/mcp_agent/app.py also detects AppContext parameters.
Description
Remove reference to internal app_ctx argument
Summary by CodeRabbit
Improvements
Documentation