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

Fixed metadata injection for local actions #1247

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mkrjn99
Copy link

@mkrjn99 mkrjn99 commented Feb 5, 2025

Important

Adds metadata injection support for local actions in ComposioToolSet and tests it with a new test case.

  • Behavior:
    • Adds add_local_action_metadata() in ProcessorHelper to handle metadata for local actions.
    • Updates execute_action() in ComposioToolSet to use add_local_action_metadata() for local actions.
  • Testing:
    • Adds test_metadata_injection.py to verify metadata injection for local actions using ComposioToolSet.

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

Copy link

vercel bot commented Feb 5, 2025

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 Feb 13, 2025 11:13pm

Copy link

LGTM 👍

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. Reviewed everything up to 29e6856 in 1 minute and 35 seconds

More details
  • Looked at 76 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. python/tests/test_tools/test_metadata_injection.py:28
  • Draft comment:
    The test only checks that the injected metadata contains the key 'api_key'. Consider testing edge cases (e.g. when metadata is None or already set) and verifying that no unintended metadata gets merged.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
2. python/composio/tools/toolset.py:399
  • Draft comment:
    In the new add_local_action_metadata method, only the metadata for the action type is injected (without app-level metadata as in add_metadata). Consider adding a comment to document this intentional behavior.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
3. python/composio/tools/toolset.py:1817
  • Draft comment:
    Storing the original action type before conversion ensures correct metadata lookup for local actions. Verify that using the unmodified action as the metadata key aligns with your metadata configuration.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
4. python/tests/test_tools/test_metadata_injection.py:28
  • Draft comment:
    The test validates metadata injection for local actions. Consider adding additional cases (e.g., non-empty initial metadata) to ensure proper merging behavior.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None

Workflow ID: wflow_aSaMY2MMUGWH0f7O


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.

@@ -1836,6 +1842,12 @@ def execute_action(
action=action,
Copy link
Contributor

Choose a reason for hiding this comment

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

The new add_local_action_metadata seems to duplicate parts of add_metadata. In execute_action, local actions get metadata injected first via add_local_action_metadata and then via add_metadata. Is this double injection intentional? Consider conditionally skipping add_metadata for local actions.

class ResponseClass(BaseModel):
pass

metadata_value = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using a more descriptive name for the global variable and adding type hints:

test_captured_metadata: Optional[str] = None  # Used to capture metadata in test scenarios

This makes the test code more maintainable and its purpose clearer.

return ResponseClass()


def test_metadata_injection() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding more test cases to cover different scenarios:

  1. Test with None metadata
  2. Test with existing metadata to ensure proper merging
  3. Test with different action types

This would improve test coverage and ensure robustness of the metadata injection functionality.

@shreysingla11
Copy link
Collaborator

Code Review Summary

Overall, the changes look well-structured and properly implement metadata injection for local actions. Here's a breakdown of the review:

Strengths:

  • Clean implementation following existing patterns
  • Good separation of concerns with dedicated method for local action metadata
  • Basic test coverage with specific test case
  • Type safety maintained with proper type hints

Suggestions for Improvement:

  1. Documentation:

    • Add docstring to add_local_action_metadata method
    • Consider adding inline comments explaining the metadata injection flow
  2. Testing:

    • Add more test cases covering edge cases and different scenarios
    • Improve test variable naming for clarity
    • Add type hints to test variables
  3. Code Structure:

    • Consider adding validation for action_type parameter
    • Consider adding debug logging for metadata injection process

The changes are good to merge after addressing the documentation and test improvements. The core functionality appears solid and well-implemented.

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.

3 participants