feat: Implement lockfiles for composio toolset #1196
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
python/composio/tools/toolset.py
Outdated
| lockfile_path = Path("./.composio.lock") | ||
| if not lockfile_path.exists(): | ||
| return {} | ||
|
|
||
| with open(lockfile_path, "r") as file: | ||
| tool_versions = yaml.safe_load(file) | ||
|
|
||
| # Validate lockfile | ||
| if not isinstance(tool_versions, dict): | ||
| raise ComposioSDKError( | ||
| f"Invalid lockfile format, expected dict, got {type(tool_versions)}" | ||
| ) | ||
|
|
||
| for tool in tool_versions.values(): | ||
| if not isinstance(tool, str): | ||
| raise ComposioSDKError( | ||
| f"Invalid lockfile format, expected version to be string, got {tool!r}" | ||
| ) | ||
|
|
||
| return tool_versions | ||
|
|
||
| def store_tool_versions_to_lockfile(self) -> None: | ||
| """Store tool versions to lockfile.""" | ||
| lockfile_path = Path("./.composio.lock") | ||
| with open(lockfile_path, "w") as file: | ||
| yaml.safe_dump(self._tool_versions, file) |
There was a problem hiding this comment.
Hardcoded lockfile path can cause write failures or security issues; use a configurable path and validate directory existence. Additionally, validate the lockfile path to prevent potential file path injection vulnerabilities.
📝 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.
| lockfile_path = Path("./.composio.lock") | |
| if not lockfile_path.exists(): | |
| return {} | |
| with open(lockfile_path, "r") as file: | |
| tool_versions = yaml.safe_load(file) | |
| # Validate lockfile | |
| if not isinstance(tool_versions, dict): | |
| raise ComposioSDKError( | |
| f"Invalid lockfile format, expected dict, got {type(tool_versions)}" | |
| ) | |
| for tool in tool_versions.values(): | |
| if not isinstance(tool, str): | |
| raise ComposioSDKError( | |
| f"Invalid lockfile format, expected version to be string, got {tool!r}" | |
| ) | |
| return tool_versions | |
| def store_tool_versions_to_lockfile(self) -> None: | |
| """Store tool versions to lockfile.""" | |
| lockfile_path = Path("./.composio.lock") | |
| with open(lockfile_path, "w") as file: | |
| yaml.safe_dump(self._tool_versions, file) | |
| def get_tool_versions_from_lockfile(self, lockfile_path_str: str) -> dict: | |
| """Get tool versions from lockfile.""" | |
| try: | |
| lockfile_path = Path(lockfile_path_str) | |
| except TypeError: | |
| raise ComposioSDKError("Invalid lockfile path provided.") | |
| if not lockfile_path.is_absolute(): | |
| lockfile_path = Path.cwd() / lockfile_path | |
| if not lockfile_path.exists(): | |
| return {} | |
| if not lockfile_path.is_file(): | |
| raise ComposioSDKError("Lockfile path is not a file.") | |
| with open(lockfile_path, "r") as file: | |
| tool_versions = yaml.safe_load(file) | |
| # Validate lockfile | |
| if not isinstance(tool_versions, dict): | |
| raise ComposioSDKError( | |
| f"Invalid lockfile format, expected dict, got {type(tool_versions)}" | |
| ) | |
| for tool in tool_versions.values(): | |
| if not isinstance(tool, str): | |
| raise ComposioSDKError( | |
| f"Invalid lockfile format, expected version to be string, got {tool!r}" | |
| ) | |
| return tool_versions | |
| def store_tool_versions_to_lockfile(self, lockfile_path_str: str) -> None: | |
| """Store tool versions to lockfile.""" | |
| try: | |
| lockfile_path = Path(lockfile_path_str) | |
| except TypeError: | |
| raise ComposioSDKError("Invalid lockfile path provided.") | |
| if not lockfile_path.is_absolute(): | |
| lockfile_path = Path.cwd() / lockfile_path | |
| # Ensure the directory exists | |
| lockfile_path.parent.mkdir(parents=True, exist_ok=True) | |
| with open(lockfile_path, "w") as file: | |
| yaml.safe_dump(self._tool_versions, file) |
python/composio/tools/toolset.py
Outdated
| with open(lockfile_path, "r") as file: | ||
| tool_versions = yaml.safe_load(file) |
There was a problem hiding this comment.
YAML.safe_load() without exception handling can crash on malformed files; add try-except for graceful error handling.
📝 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.
| with open(lockfile_path, "r") as file: | |
| tool_versions = yaml.safe_load(file) | |
| try: | |
| with open(lockfile_path, "r") as file: | |
| tool_versions = yaml.safe_load(file) | |
| except yaml.YAMLError as e: | |
| print(f"Error loading YAML file: {e}") | |
| tool_versions = None |
python/composio/tools/toolset.py
Outdated
| lockfile_path = Path("./.composio.lock") | ||
| with open(lockfile_path, "w") as file: | ||
| yaml.safe_dump(self._tool_versions, file) |
There was a problem hiding this comment.
File operations lack proper closing in error cases; use context managers consistently for cleanup.
📝 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.
| lockfile_path = Path("./.composio.lock") | |
| with open(lockfile_path, "w") as file: | |
| yaml.safe_dump(self._tool_versions, file) | |
| try: | |
| with open(lockfile_path, "w") as file: | |
| yaml.safe_dump(self._tool_versions, file) | |
| except Exception as e: | |
| print("Error writing to lockfile:", e) |
| for schema in action_schemas: | ||
| schema["name"] = schema["enum"] | ||
|
|
||
| # TODO: Put version in local tool schemas | ||
| schema["version"] = "0_1" |
There was a problem hiding this comment.
The hardcoded version string '0_1' should be dynamically fetched to ensure accuracy and consistency.
📝 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.
| for schema in action_schemas: | |
| schema["name"] = schema["enum"] | |
| # TODO: Put version in local tool schemas | |
| schema["version"] = "0_1" | |
| for schema in action_schemas: | |
| schema["name"] = schema["enum"] | |
| # TODO: Put version in local tool schemas | |
| schema["version"] = get_version() | |
python/composio/tools/toolset.py
Outdated
| @staticmethod | ||
| def load_tool_versions_from_lockfile() -> t.Dict[str, str]: | ||
| """Load tool versions from lockfile.""" | ||
| lockfile_path = Path("./.composio.lock") |
There was a problem hiding this comment.
The lockfile path is hardcoded as ./.composio.lock. Consider using a constant or configuration value for this path, similar to how other paths are handled in the codebase (e.g., LOCAL_CACHE_DIRECTORY). This would make it easier to change the location if needed and maintain consistency with other file paths in the codebase.
python/composio/tools/toolset.py
Outdated
| if not lockfile_path.exists(): | ||
| return {} | ||
|
|
||
| with open(lockfile_path, "r") as file: |
There was a problem hiding this comment.
The file operations here lack proper error handling. Consider wrapping the file operations in try-except blocks to handle potential IOError, PermissionError, or yaml.YAMLError exceptions. This would provide better error messages and prevent crashes in case of file access or parsing issues.
![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 a2b7d3f in 2 minutes and 11 seconds
More details
- Looked at
174lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. python/composio/tools/base/abs.py:344
- Draft comment:
Setting a hardcoded version '0_1' is not ideal. Consider fetching the version dynamically from an API or configuration. - Reason this comment was not posted:
Confidence changes required:50%
The code is setting a default version '0_1' for schemas, but it should ideally fetch the version from an API or a more dynamic source. This is a potential area for improvement.
2. python/composio/tools/local/handler.py:60
- Draft comment:
Setting a hardcoded version '0_1' is not ideal. Consider fetching the version dynamically from an API or configuration. - Reason this comment was not posted:
Confidence changes required:50%
The code is setting a default version '0_1' for schemas, but it should ideally fetch the version from an API or a more dynamic source. This is a potential area for improvement.
3. python/composio/tools/toolset.py:1067
- Draft comment:
Setting a hardcoded version '0_1' is not ideal. Consider fetching the version dynamically from an API or configuration. - Reason this comment was not posted:
Confidence changes required:50%
The code is setting a default version '0_1' for schemas, but it should ideally fetch the version from an API or a more dynamic source. This is a potential area for improvement.
Workflow ID: wflow_hjfafTpMeXcr6IXK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -56,6 +56,8 @@ def get_action_schemas( | |||
| for schema in action_schemas: | |||
| schema["name"] = schema["enum"] | |||
|
|
|||
| # TODO: Put version in local tool schemas | |||
| schema["version"] = "0_1" | |||
There was a problem hiding this comment.
There's a potential bug in the get_action_schemas method in handler.py where schema["version"] is set after the loop. This should be inside the loop as it's modifying a single schema instead of all schemas in the list.
Code Review SummaryThe implementation of lockfiles for the composio toolset is a good addition that will help with version management. However, there are several areas that need attention: Critical Issues:
Improvements Needed:
Positive Aspects:
Overall Rating: 7/10 - Good implementation but needs addressing the identified issues for better reliability and maintainability. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9d3823c in 45 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/plugins/autogen/composio_autogen/toolset.py:259
- Draft comment:
The removal of theself.locking_enabledcheck and the associated logic for updating_tool_versionsmight affect tool version management. Iflocking_enabledis intended to control this behavior, consider re-adding this logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative, saying "might affect" and using "if" conditions. There's no clear evidence in the file that removing this logic is problematic. The variables aren't used elsewhere in the file. The comment is asking the author to "consider" something rather than pointing out a definite issue.
I could be missing important context about tool version management that exists in other files or in the broader codebase. The locking feature could be important for version control.
Even if version locking is important, the comment is still speculative and asks for consideration rather than pointing out a definite issue. Per the rules, we should not keep speculative comments or comments that require broader context to understand.
Delete the comment because it is speculative ("might affect") and asks the author to "consider" something rather than pointing out a definite issue.
Workflow ID: wflow_amo5qAfiSLOwFHjV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
python/composio/tools/toolset.py
Outdated
| def store_tool_versions_to_lockfile(self) -> None: | ||
| """Store tool versions to lockfile.""" | ||
| lockfile_path = Path("./.composio.lock") | ||
| with open(lockfile_path, "w", encoding="utf-8") as file: |
There was a problem hiding this comment.
The 'encoding' parameter should be explicitly set to 'utf-8' when opening files to prevent encoding issues.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8ebb3c2 in 25 seconds
More details
- Looked at
51lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/plugins/autogen/composio_autogen/toolset.py:245
- Draft comment:
The change in theget_toolsfunction improves readability by assigning the list comprehension to a variable before returning it. This is a good practice for complex expressions. - Reason this comment was not posted:
Confidence changes required:0%
The change in theget_toolsfunction is a minor refactor that doesn't affect functionality. It improves readability by assigning the list comprehension to a variable before returning it.
Workflow ID: wflow_09P5KqISSBrbN5Gj
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 64afacb in 42 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/local/handler.py:58
- Draft comment:
The TODO comment is outdated and should be removed since the version is now being set in the loop. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is technically incorrect. While the version assignment was moved inside the loop, the TODO comment is still relevant - it's asking to "Put version in local tool schemas" which hasn't been fully addressed just by moving the assignment inside the loop. The TODO appears to be about a larger architectural issue of where versions should be defined for local tools.
Maybe I'm misinterpreting the TODO's intent - perhaps it was just about the mechanical act of setting the version field?
No, the TODO's wording specifically mentions "in local tool schemas" which suggests it's about where the version information should be architecturally stored, not just about setting the field.
The comment should be deleted because it incorrectly assumes the TODO is no longer relevant just because the version assignment moved location.
Workflow ID: wflow_PZQAyz5MMDv54odv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @staticmethod | ||
| def load_tool_versions_from_lockfile() -> t.Dict[str, str]: | ||
| """Load tool versions from lockfile.""" | ||
| if not LOCKFILE_PATH.exists(): | ||
| return {} | ||
|
|
||
| with open(LOCKFILE_PATH, encoding="utf-8") as file: | ||
| tool_versions = yaml.safe_load(file) | ||
|
|
||
| # Validate lockfile | ||
| if not isinstance(tool_versions, dict): | ||
| raise ComposioSDKError( | ||
| f"Invalid lockfile format, expected dict, got {type(tool_versions)}" | ||
| ) | ||
|
|
||
| for tool in tool_versions.values(): | ||
| if not isinstance(tool, str): | ||
| raise ComposioSDKError( | ||
| f"Invalid lockfile format, expected version to be string, got {tool!r}" | ||
| ) | ||
|
|
||
| return tool_versions | ||
|
|
||
| def store_tool_versions_to_lockfile(self) -> None: | ||
| """Store tool versions to lockfile.""" | ||
| with open(LOCKFILE_PATH, "w", encoding="utf-8") as file: | ||
| yaml.safe_dump(self._tool_versions, file) | ||
|
|
There was a problem hiding this comment.
Opening files with write mode without proper error handling could fail silently or leave files in an inconsistent state. Add try-except to handle IOError/OSError when reading/writing lockfile.
python/composio/tools/toolset.py
Outdated
| LOCAL_CACHE_DIRECTORY, | ||
| LOCAL_OUTPUT_FILE_DIRECTORY_NAME, | ||
| USER_DATA_FILE_NAME, | ||
| LOCKFILE_PATH, |
There was a problem hiding this comment.
The LOCKFILE_PATH variable is defined but not imported in the toolset.py file, which may cause a NameError during execution.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 01412ef in 16 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/constants.py:102
- Draft comment:
Consider using an absolute path forLOCKFILE_PATHto avoid issues with changing working directories. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a constant for the lockfile path and uses it in multiple places. This is a good practice as it centralizes the path definition, making it easier to change in the future if needed. However, the constant is defined using a relative path, which can lead to issues if the working directory changes. It's better to use an absolute path or ensure the working directory is consistent.
Workflow ID: wflow_DhAFSay2AUriao6B
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 be4984c in 16 seconds
More details
- Looked at
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/toolset.py:54
- Draft comment:
Consider keeping imports organized, typically with constants grouped together. - Reason this comment was not posted:
Confidence changes required:33%
The import order was changed, but it doesn't affect functionality. However, it is a good practice to keep imports organized.
Workflow ID: wflow_608lSHgOym91Wz5M
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| self._tool_versions = { | ||
| tool.name: tool.version for tool in items | ||
| } | self._tool_versions |
There was a problem hiding this comment.
The change in merge order means new tool versions will take precedence over existing ones, potentially overwriting intended version locks. Consider if this new precedence is desired.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 33d6814 in 20 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/tools/toolset.py:1105
- Draft comment:
The order of merging dictionaries has been changed. Previously,self._tool_versionswas updated with new tool versions, but now the new tool versions will overwrite existing ones if there are conflicts. Ensure this is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_079WKxlcHjcMiQPw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
This PR adds lockfile functionality to the Composio toolset for managing tool versions, including a new
versionattribute inActionModeland methods for lockfile operations.versionattribute toActionModelincollections.py.toolset.pyto manage tool versions.LOCKFILE_PATHconstant inconstants.py.load_tool_versions_from_lockfile()andstore_tool_versions_to_lockfile()intoolset.pyfor handling lockfile operations._generate_schema()inabs.pyandget_action_schemas()inhandler.py.This description was created by
for 33d6814. It will automatically update as commits are pushed.