Skip to content
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

fix: Only fetch actions available to an entity during schema validation #1095

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Dec 27, 2024

Important

Add entity_id to check_connected_account for entity-specific validation in toolset.py and set default in ConnectedAccountModel.

  • Behavior:
    • Add entity_id parameter to check_connected_account in toolset.py for entity-specific action validation.
    • Modify _execute_remote and get_action_schemas in toolset.py to use entity_id.
  • Models:
    • Set entityId to DEFAULT_ENTITY_ID in ConnectedAccountModel in collections.py.
  • Tests:
    • Minor adjustment in test_example.py to restore original file content after test execution.

This description was created by Ellipsis for e71c298. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 0:40am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 9fe5466 in 15 seconds

More details
  • Looked at 39 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/tools/toolset.py:435
  • Draft comment:
    Consider refreshing _connected_accounts to ensure it reflects the latest state, especially after filtering by entity_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.

@shreysingla11
Copy link
Collaborator

Code Review Summary

The 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
✅ Consistent updates across all usage points
✅ Maintains backward compatibility with existing auth checks

Suggestions for Improvement:

  • Update docstring to document the new entity_id parameter
  • Enhance error messages to include entity context
  • Consider adding debug logging for troubleshooting

Code Quality: 8/10

The core changes are solid and improve security, with minor documentation improvements needed.

The PR is ready to merge after addressing the documentation suggestions.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 3fda8b6 in 41 seconds

More details
  • Looked at 38 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. python/composio/client/collections.py:94
  • Draft comment:
    Setting a default value for entityId in ConnectedAccountModel might 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:
  1. This is a Pydantic model for connected accounts
  2. DEFAULT_ENTITY_ID appears to be a constant imported specifically for this purpose
  3. The change makes the field required with a default rather than optional
  4. This seems like an intentional design choice to ensure entityId is always set
  5. 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 of entity_id parameter in check_connected_account ensures 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 of toolset.py passes self.entity_id to check_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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on e71c298 in 39 seconds

More details
  • Looked at 37 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted 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.

Comment on lines 33 to 39
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_ID is 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.


Comment on lines 91 to 97
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@ComposioHQ ComposioHQ deleted a comment from shreysingla11 Jan 24, 2025
@ComposioHQ ComposioHQ deleted a comment from shreysingla11 Jan 24, 2025
@ComposioHQ ComposioHQ deleted a comment from ellipsis-dev bot Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants