fix: Only fetch actions available to an entity during schema validation #1095
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 9fe5466 in 15 seconds
More details
- Looked at
39lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/toolset.py:435
- Draft comment:
Consider refreshing_connected_accountsto ensure it reflects the latest state, especially after filtering byentity_id. This prevents potential issues with stale data. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_El7CN0bfStJNmb7r
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Code Review SummaryThe changes look good overall and improve security by adding entity-specific validation for connected accounts. Here's a brief assessment: Strengths:✅ Proper entity-specific validation of connected accounts Suggestions for Improvement:
Code Quality: 8/10The core changes are solid and improve security, with minor documentation improvements needed. The PR is ready to merge after addressing the documentation suggestions. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3fda8b6 in 41 seconds
More details
- Looked at
38lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/client/collections.py:94
- Draft comment:
Setting a default value forentityIdinConnectedAccountModelmight lead to unexpected behavior if not intended for all instances. Consider handling defaults explicitly where needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests being cautious about setting default values. However, looking at the code:
- This is a Pydantic model for connected accounts
- DEFAULT_ENTITY_ID appears to be a constant imported specifically for this purpose
- The change makes the field required with a default rather than optional
- This seems like an intentional design choice to ensure entityId is always set
- There's no evidence this would cause issues
I could be missing context about how this model is used in practice. The default value could potentially mask bugs where entityId should be explicitly set.
The change from optional to required with default appears intentional and follows good practices of being explicit. The imported constant suggests this is the intended behavior.
The comment should be deleted as it questions an intentional design choice without strong evidence of any actual issues.
2. python/composio/tools/toolset.py:902
- Draft comment:
The addition ofentity_idparameter incheck_connected_accountensures entity-specific validation, aligning with the PR description. No issues here. - Reason this comment was not posted:
Confidence changes required:0%
The change in line 902 oftoolset.pypassesself.entity_idtocheck_connected_account. This is consistent with the PR description and ensures entity-specific validation. No issues here.
Workflow ID: wflow_xQvcvs5N0cFcfR7T
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e71c298 in 39 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_186vXRU5cQwnmntH
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.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| TriggerType, | ||
| ) | ||
| from composio.client.exceptions import ComposioClientError, ComposioSDKError | ||
| from composio.constants import PUSHER_CLUSTER, PUSHER_KEY | ||
| from composio.constants import DEFAULT_ENTITY_ID, PUSHER_CLUSTER, PUSHER_KEY | ||
| from composio.utils import help_msg, logging | ||
| from composio.utils.shared import generate_request_id | ||
|
|
There was a problem hiding this comment.
Potential Issue: The addition of DEFAULT_ENTITY_ID to the import statement indicates a change in how entity IDs are managed across the codebase. This could potentially introduce a default value where it was previously optional or unspecified, affecting the behavior of entity-related operations.
Actionable Steps:
- Review Usage: Check all instances where
DEFAULT_ENTITY_IDis used to ensure it aligns with the intended logic and does not introduce unintended defaults. - Test Coverage: Ensure that test cases cover scenarios with and without the default entity ID to prevent regressions.
- Documentation: Update any relevant documentation to reflect the introduction of a default entity ID, if applicable.
This change could have a broader impact on the system's behavior, especially if entity IDs are critical to the application's logic.
| connectionParams: AuthConnectionParamsModel | ||
|
|
||
| clientUniqueUserId: t.Optional[str] = None | ||
| entityId: t.Optional[str] = None | ||
| entityId: str = DEFAULT_ENTITY_ID | ||
|
|
||
| # Override arbitrary model config. | ||
| model_config: ConfigDict = ConfigDict( # type: ignore |
There was a problem hiding this comment.
Refactor: The change from an optional entityId to a default value of DEFAULT_ENTITY_ID enhances consistency across the codebase. This aligns with the check_connected_account function, which now expects an entity_id parameter. This change ensures logical consistency and reduces potential errors related to missing entityId values.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2a33873 in 28 seconds
More details
- Looked at
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/tools/toolset.py:478
- Draft comment:
The filter condition replaced connection.entityId with connection.clientUniqueUserId. Confirm that this is intended and that no backward compatibility is needed if some accounts might use the old field. - Reason this comment was not posted:
Comment did not seem useful: The comment asks the PR author to confirm their intention and check for backward compatibility, which violates the rules. It does not provide a specific suggestion or question about the code itself.
2. python/composio/tools/toolset.py:478
- Draft comment:
Usingconnection.clientUniqueUserIdinstead ofconnection.entityIdis appropriate given the API response. Ensure this property is consistently available to avoid breaking existing integrations. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Aq1IQRvyQaf7lvSn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
python/composio/tools/toolset.py
Outdated
| if action.app not in [ | ||
| connection.appUniqueId.upper() # Normalize app names/ids coming from API | ||
| for connection in self._connected_accounts | ||
| if connection.clientUniqueUserId == entity_id | ||
| ]: |
There was a problem hiding this comment.
The check_connected_account method now filters connections by clientUniqueUserId but doesn't handle the case when entity_id is None, which could incorrectly reject valid connections. Should add a conditional check.
📝 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 action.app not in [ | |
| connection.appUniqueId.upper() # Normalize app names/ids coming from API | |
| for connection in self._connected_accounts | |
| if connection.clientUniqueUserId == entity_id | |
| ]: | |
| if action.app not in [ | |
| connection.appUniqueId.upper() # Normalize app names/ids coming from API | |
| for connection in self._connected_accounts | |
| if entity_id is None or connection.clientUniqueUserId == entity_id | |
| ]: |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on fefebc2 in 1 minute and 53 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/tools/toolset.py:1681
- Draft comment:
New parameter ‘entity_id’ is used to filter connected accounts. Consider defaulting to self.entity_id if entity_id is None, to avoid potential mismatches when not explicitly provided. - Reason this comment was not posted:
Marked as duplicate.
2. python/composio/tools/toolset.py:1793
- Draft comment:
Updating the call to check_connected_account in _execute_remote ensures the correct entity_id is passed. Confirm that entity_id is always properly provided to avoid filtering out valid connections. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the author to confirm that the entity_id is always properly provided, which violates the rule against asking the author to confirm their intention or ensure behavior. However, it does point out a potential issue with the entity_id being filtered out, which could be useful. I need to decide if the comment is more about asking for confirmation or pointing out a potential issue.
Workflow ID: wflow_iCHvVjXaSJu6USA9
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.
| @@ -1659,7 +1659,9 @@ def _validate_connection_ids( | |||
| return valid | |||
| raise InvalidConnectedAccount(f"Invalid connected accounts found: {invalid}") | |||
|
|
|||
| def check_connected_account(self, action: ActionType) -> None: | |||
| def check_connected_account( | |||
There was a problem hiding this comment.
The new parameter entity_id is added to check_connected_account but the docstring hasn’t been updated. Please document the new parameter.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e193bfd in 57 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/tests/test_example.py:243
- Draft comment:
Good improvement with error message, but consider using proc.communicate() to capture both stdout and stderr instead of directly reading from proc.stderr, which can help avoid potential deadlocks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While proc.communicate() is generally a good practice to avoid deadlocks when reading from subprocess pipes, in this case the code is already safe because: 1) proc.wait() is called first with a timeout 2) stderr is only read after the process has completed 3) the process has already terminated when we read stderr. The current implementation is actually fine and there's no real risk of deadlock.
The comment raises a valid general best practice. In some subprocess scenarios, not using communicate() can cause deadlocks if buffers fill up while reading output.
While communicate() is a good practice in general, in this specific case the code is already safe because we only read stderr after the process has fully completed with wait().
Delete this comment. While the suggestion follows a good general practice, the current implementation is already safe from deadlocks due to its structure, making this change unnecessary.
2. python/tests/test_example.py:243
- Draft comment:
Nice improvement: providing stderr output in the assert message aids in debugging. Ensure proc.stderr is always set (it is here via PIPE). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_VuZXqARLdfHFOkog
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 419aa66 in 1 minute and 22 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. python/tests/test_example.py:218
- Draft comment:
Removed reading of original_source. Confirm that not reverting file changes is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code modifies test files by adding Helicone headers during test runs. Previously it would restore the files afterward, but now it doesn't. This could potentially be an issue if the modified files get committed. However, these are test files and the modification is temporary during test execution. The build system likely has clean checkout mechanisms. Without more context about the testing infrastructure, I can't be certain this is actually problematic.
I may be underestimating the impact of not restoring test files. There could be subsequent tests that depend on the original file contents.
While valid, test isolation is typically handled at a higher level through test runners and CI systems. The fact that this change was made deliberately suggests the team knows their test infrastructure.
While the comment identifies a real change in behavior, there isn't strong evidence that this change is problematic. Without more context about the test infrastructure, we should trust the author's judgment.
2. python/tests/test_example.py:237
- Draft comment:
Removed file restoration after test; ensure file modifications are acceptable or revert using a try/finally. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. python/tests/test_example.py:218
- Draft comment:
Removal of the original_source backup means the test no longer restores the example file’s content, which may lead to persistent modifications. Consider using a temporary copy or a teardown step. - Reason this comment was not posted:
Marked as duplicate.
4. python/tests/test_example.py:237
- Draft comment:
Removal of the file restoration (write_text with original_source) can leave modified files after test execution. Using a temporary file or restoring state in a finally clause may be safer. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Cz2Rjvn7xBzDVEvg
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 5722fd9 in 1 minute and 51 seconds
More details
- Looked at
39lines of code in2files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. python/composio/tools/toolset.py:1680
- Draft comment:
Check that bypassing the entity check when entity_id is None is intended. A clarifying comment might help. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is asking for verification of intended behavior and suggesting documentation. This violates the rule "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended". The behavior seems logical - when no entity_id is provided, check all connected accounts. This is a reasonable default. The code change is clear and the logic makes sense.
The behavior of bypassing entity filtering when entity_id is None could be non-obvious to future readers. A clarifying comment could improve code maintainability.
While documentation could help, we should not ask authors to verify their intentions or add comments unless absolutely necessary. The code logic is straightforward enough to understand from reading it.
Delete this comment. It violates the rule about asking authors to verify intentions, and the code behavior is clear enough without additional comments.
2. python/tests/test_example.py:221
- Draft comment:
Good practice storing and restoring the original source text, but consider using a try/finally block to guarantee restoration in case of test failures. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. python/tests/test_example.py:236
- Draft comment:
Removing the detailed stderr message in the assertion may hinder debugging; consider logging stderr if the test fails. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. python/composio/tools/toolset.py:1679
- Draft comment:
When 'entity_id' is None, the condition now bypasses filtering. Confirm this is the intended behavior and consider updating the docstring to clarify. - Reason this comment was not posted:
Marked as duplicate.
5. python/tests/test_example.py:242
- Draft comment:
Removed detailed stderr output from the assert; including error output can help diagnose failures. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide a specific suggestion or question for the PR author. It simply states that detailed stderr output was removed and suggests that including error output can help diagnose failures, but it doesn't ask for any action or clarification.
6. python/composio/tools/toolset.py:1905
- Draft comment:
Typo: The method name 'process_respone' should be corrected to 'process_response' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. python/composio/tools/toolset.py:357
- Draft comment:
Typographical error in the error message: "Schema pprocessors cannot be retried." should be corrected to "Schema processors cannot be retried." - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. python/composio/tools/toolset.py:1690
- Draft comment:
Typographical error in the docstring: 'current entiry' should be corrected to 'current entity'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. python/composio/tools/toolset.py:1171
- Draft comment:
Typographical error in the docstring: 'suppossed' should be corrected to 'supposed'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_IPJQufsS8B0J5kGg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Enhance schema validation by filtering actions based on entity availability and update tests to prevent side effects.
check_connected_accountintoolset.pyto acceptentity_idand filter connected accounts byentity_id.ConnectedAccountModelincollections.pyto setentityIdtoDEFAULT_ENTITY_ID._execute_remoteintoolset.pyto passentity_idtocheck_connected_account.test_example.py, restore original source code after test execution to prevent side effects.This description was created by
for 5722fd9. It will automatically update as commits are pushed.