-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: add missed app_id for workflow #180
Conversation
WalkthroughThe pull request introduces an optional Changes
Possibly related issues
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🔭 Outside diff range comments (1)
cozepy/workflows/runs/__init__.py (1)
Line range hint
332-337
: Addapp_id
parameter to async stream method.The async version of the stream method is missing the
app_id
parameter, which would cause it to fail with the same "App not found" error. The sync and async implementations should maintain parity.Apply this diff to add the parameter and update the body:
async def stream( self, *, workflow_id: str, parameters: Optional[Dict[str, Any]] = None, bot_id: Optional[str] = None, + app_id: Optional[str] = None, ext: Optional[Dict[str, Any]] = None, ) -> AsyncIterator[WorkflowEvent]: url = f"{self._base_url}/v1/workflow/stream_run" body = { "workflow_id": workflow_id, "parameters": parameters, "bot_id": bot_id, + "app_id": app_id, "ext": ext, }Don't forget to add the parameter documentation to the docstring as well.
🧹 Nitpick comments (1)
cozepy/workflows/runs/__init__.py (1)
202-203
: Add documentation for theapp_id
parameter.The
app_id
parameter is missing from the method's docstring. Please add the parameter documentation to maintain consistency with other parameters.Add this to the docstring:
:param bot_id: The associated Bot ID required for some workflow executions, such as workflows with database nodes, variable nodes, etc. + :param app_id: The ID of the application to execute the workflow under. :param ext: Used to specify some additional fields in the format of Map[String][String].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cozepy/workflows/runs/__init__.py
(2 hunks)
🔇 Additional comments (1)
cozepy/workflows/runs/__init__.py (1)
202-203
: Verify app_id requirements from API documentation.Please verify the following from the API documentation:
- Are there any specific format requirements for app_id?
- Is app_id required for specific workflow types?
- Are there any constraints or limitations when using app_id?
This information would be valuable for adding proper validation and documentation.
Also applies to: 223-223
✅ Verification successful
The app_id parameter is properly documented and implemented
The codebase shows consistent usage of
app_id
:
- It's documented as "The ID of the app that the API interacts with"
- Implementation accepts hex string values (as shown in tests)
- Type hints are correctly used (Optional[str] for workflows)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if app_id requirements are documented in README or API docs # Look for app_id documentation in markdown files echo "Searching for app_id documentation..." rg -i "app.?id" --type md -C 3 # Look for app_id validation in Python files echo "Checking for existing app_id validation..." rg -i "app.?id.*validation|validate.*app.?id" --type py -C 3Length of output: 275
Script:
#!/bin/bash # Search for app_id usage patterns in Python files echo "Searching for app_id usage in Python files..." rg -i "app_id" --type py -C 2 # Search for test files containing app_id echo "Searching for app_id in test files..." rg -i "app_id" -g "*test*.py" -C 2 # Search for function definitions with app_id parameter echo "Searching for function definitions with app_id..." ast-grep --pattern 'def $_(self, $$$, app_id: $_,$$$):'Length of output: 7289
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #180 +/- ##
=======================================
Coverage 90.01% 90.01%
=======================================
Files 65 65
Lines 5840 5840
=======================================
Hits 5257 5257
Misses 583 583
|
Pretty straight forward change. Currently the demo code (https://www.coze.com/open/playground/workflow_run), Python section, gives an error:
got error error_code=4000 error_message='App not found. Please verify the app_id is correct and ensure the app has been published.'
Summary by CodeRabbit
app_id
parameter to workflow streaming methods, providing more flexibility in specifying application context for workflow execution.