Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 7b1061b in 21 seconds
More details
- Looked at
132lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. python/composio/cli/add.py:246
- Draft comment:
Ensureauth_modeis checked forNonebefore callinglower()to avoid potentialAttributeError. This is already done in the PR, but ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a change whereauth_modeis checked forNonebefore callinglower(). This is a good practice to avoid potentialAttributeErrorifauth_modeisNone. However, the same check should be applied consistently across the codebase whereauth_modeis used.
2. python/composio/client/collections.py:373
- Draft comment:
Ensureauth_scheme.modeis a validAuthSchemeTypebefore casting to avoid potential runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes theAppAuthSchemeclass to useOptionalfor several fields, which is a good practice for fields that may not always have a value. However, themodefield is being cast toAuthSchemeTypewithout checking if it is a valid value. This could lead to runtime errors ifmodeis not a validAuthSchemeType.
3. python/composio/tools/toolset.py:1062
- Draft comment:
Ensureauth_schemeis checked forNonebefore callingupper()to avoid potentialAttributeError. This is already done in the PR, but ensure consistency across the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a change whereauth_modeis checked forNonebefore callingupper(). This is a good practice to avoid potentialAttributeErrorifauth_modeisNone. However, the same check should be applied consistently across the codebase whereauth_modeis used.
Workflow ID: wflow_P1g94F501tQcPIzY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| apps: t.Union[AppModel, t.List[AppModel]] = super().get(queries=queries) | ||
| for app in apps: | ||
| if app.auth_schemes is not None: # type: ignore | ||
| for auth_scheme in app.auth_schemes: # type: ignore | ||
| if auth_scheme.mode is not None: | ||
| auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode) | ||
| return apps |
There was a problem hiding this comment.
Type casting apps to AppModel before iteration can raise TypeError since apps could be a single AppModel. Should check type and handle single model case.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| apps: t.Union[AppModel, t.List[AppModel]] = super().get(queries=queries) | |
| for app in apps: | |
| if app.auth_schemes is not None: # type: ignore | |
| for auth_scheme in app.auth_schemes: # type: ignore | |
| if auth_scheme.mode is not None: | |
| auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode) | |
| return apps | |
| apps: t.Union[AppModel, t.List[AppModel]] = super().get(queries=queries) | |
| if isinstance(apps, AppModel): | |
| if apps.auth_schemes is not None: | |
| for auth_scheme in apps.auth_schemes: | |
| if auth_scheme.mode is not None: | |
| auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode) | |
| else: | |
| for app in apps: | |
| if app.auth_schemes is not None: | |
| for auth_scheme in app.auth_schemes: | |
| if auth_scheme.mode is not None: | |
| auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode) | |
| return apps |
python/composio/tools/toolset.py
Outdated
| if scheme.auth_mode is not None: | ||
| if auth_scheme != scheme.auth_mode.upper(): | ||
| continue |
There was a problem hiding this comment.
Potential NoneType error when accessing auth_mode - need to check if scheme itself is not None before accessing auth_mode
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if scheme.auth_mode is not None: | |
| if auth_scheme != scheme.auth_mode.upper(): | |
| continue | |
| if scheme is not None and scheme.auth_mode is not None: | |
| if auth_scheme != scheme.auth_mode.upper(): | |
| continue |
| @@ -293,8 +293,10 @@ class AuthSchemeField(BaseModel): | |||
| class AppAuthScheme(BaseModel): | |||
| """App authenticatio scheme.""" | |||
There was a problem hiding this comment.
There's a typo in the docstring: authenticatio should be authentication
| @@ -1051,8 +1058,9 @@ def get_expected_params_for_user( | |||
| # without user inputs to create an integratuib, if yes then create | |||
There was a problem hiding this comment.
There's a typo in the comment: integratuib should be integration
| scheme_name: t.Optional[str] = None | ||
| name: t.Optional[str] = None | ||
| auth_mode: t.Optional[AuthSchemeType] = None | ||
| mode: t.Optional[str] = None |
There was a problem hiding this comment.
Consider adding a docstring to explain the purpose of the mode field and its relationship with auth_mode. This would help clarify why both fields exist and when each should be used.
| additional_fields: t.List[str] = ["auth_schemes"], | ||
| ) -> t.List[AppModel]: | ||
| apps = self.client.apps.get() | ||
| # added type ignore since method overload was not being referenced |
There was a problem hiding this comment.
The type ignore comment should explain why the method overload is not being referenced and what's the proper way to fix it. This would help future maintainers understand if this is a temporary workaround or a permanent solution.
Code Review SummaryThe changes look good overall, with improvements to authentication scheme handling and type safety. Here's a breakdown: Positives:
Areas for Improvement:
The code changes are well-structured and improve the robustness of the authentication handling. The suggestions are mostly about documentation and clarity rather than functional issues. Rating: 8/10 - Solid improvements with minor documentation needs. |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 277eae9 in 36 seconds
More details
- Looked at
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Bt62BKfJgA36Xx0H
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| ComposioToolSet().get_action_schemas(actions=[Action.ATTIO_UPDATE_A_LIST]) | ||
|
|
||
|
|
||
| def test_get_apps() -> None: |
There was a problem hiding this comment.
Handle the case where no app with no_auth set to False is found to avoid potential UnboundLocalError.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8524a9e in 10 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:81
- Draft comment:
Unnecessary blank line. Consider removing it to maintain code consistency. - Reason this comment was not posted:
Confidence changes required:10%
The PR adds an unnecessary blank line which does not contribute to code readability or functionality.
Workflow ID: wflow_yOegUl0xHqvJDmk3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ec3e8a4 in 35 seconds
More details
- Looked at
92lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/toolset.py:870
- Draft comment:
Theinclude_localparameter is removed from the method signature but is still used in the method logic. Consider either removing its usage from the method or re-adding it to the method signature. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_cNrm9lqTYT4Xz3LD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
marking stale |
3 participants
fixes ENG-2491
Important
Fixes and enhances handling of authentication schemes in Composio SDK, ensuring proper validation and error handling for missing or invalid auth modes.
auth_modeinadd_integration()inadd.pyto ensure it is notNonebefore processing.get()incollections.pyto setauth_modefrommodeif available.ComposioSDKErroringet_expected_params_for_user()andfetch_expected_integration_params()intoolset.pyifauth_modeisNone.t.Optionaltoscheme_name,name,auth_mode, andmodeinAppAuthSchemeincollections.py.test_get_apps()intest_toolset.pyto verifyauth_modeis inAUTH_SCHEMES.auth_modeintest_toolset.pyto ensure proper handling of auth schemes.This description was created by
for ec3e8a4. It will automatically update as commits are pushed.