Closed utkarsh-dixit closed 1 month ago
โฑ๏ธ Estimated effort to review: 4 ๐ต๐ต๐ต๐ตโช |
๐งช No relevant tests |
๐ No security concerns identified |
โก Key issues to review Code Redundancy The PR replaces double quotes with single quotes for string literals in the `App` class. This change does not affect functionality and could be considered unnecessary unless it aligns with a specific coding standard adopted by the team. It's important to ensure that such changes are consistent across the entire codebase to maintain readability and coding standards. Hardcoded Values The output text in the `GitRepoTree` class has been updated to include a specific function name (`FILEEDITTOOL_OPEN_FILE`). This change hardcodes the function name into the output text, which could reduce flexibility if the function name changes or if different functions need to be referenced in the future. Consider using a more dynamic approach to referencing function names. Clarity in Documentation The description for the `shell_id` field in the `ShellRequest` class has been updated to clarify that it is optional. This is a positive change for clarity in the documentation. However, ensure that all related documentation and user guides are updated to reflect this change to avoid confusion. Script Complexity The new script added for a software engineer example is quite lengthy and complex, which could make it difficult to maintain and debug. Consider breaking down the script into smaller, more manageable functions or modules, especially if the script is expected to grow or evolve. |
Category | Suggestion | Score |
Best practice |
Implement a validation mechanism to check for duplicate or undefined actions___ **To ensure the integrity and correctness of the enum values, consider implementing avalidation mechanism that checks for duplicate or undefined actions during the initialization of the Action class.**
[python/composio/client/enums/_action.py [13-15]](https://github.com/ComposioHQ/composio/pull/366/files#diff-a375ebeaef8787ee1ac6fb7a8d50350b8de2041ebe2460dc05aa3de345108624R13-R15)
```diff
class Action(_AnnotatedEnum[ActionData], path=ACTIONS_CACHE):
"""Class to represent `Action` entity."""
- APIFY_CREATE_APIFY_ACTOR: 'Action'
+ _actions_set = set()
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ if self.name in self._actions_set:
+ raise ValueError(f"Duplicate action name detected: {self.name}")
+ self._actions_set.add(self.name)
+
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 10Why: Implementing a validation mechanism ensures the integrity and correctness of enum values, preventing potential runtime errors due to duplicates. | 10 |
Update the
___
**Update the | 8 | |
Pin dependency versions to specific current versions to avoid potential issues with automatic updates___ **Consider specifying a more restrictive version range for dependencies to avoidpotential breaking changes when new versions are released. Using caret (^) allows any minor version updates, which might introduce unexpected behavior.** [js/examples/swe/package.json [13-16]](https://github.com/ComposioHQ/composio/pull/366/files#diff-4b9d91b7b6b142dbd16e62e45931a1c49c1ece7c9df5675383ee499239ce4ee3R13-R16) ```diff -"@langchain/anthropic": "^0.2.6", -"@langchain/openai": "^0.2.4", -"dotenv": "^16.4.5", +"@langchain/anthropic": "0.2.6", +"@langchain/openai": "0.2.4", +"dotenv": "16.4.5", ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Pinning dependency versions can prevent unexpected behavior due to automatic updates, which is important for maintaining stability in the project. | 8 | |
Add type annotations to the
___
**To avoid potential runtime errors and improve code robustness, consider adding type | 7 | |
Verify and potentially update the license to align with project or organizational standards___ **The 'license' field is set to 'ISC', which is a permissive license. Ensure that thislicense choice is intentional and consistent with the licensing policies of your organization or personal projects, especially when integrating multiple third-party libraries.** [js/examples/swe/package.json [11]](https://github.com/ComposioHQ/composio/pull/366/files#diff-4b9d91b7b6b142dbd16e62e45931a1c49c1ece7c9df5675383ee499239ce4ee3R11-R11) ```diff -"license": "ISC", +"license": "MIT", ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: Ensuring the correct license is important for legal and compliance reasons, but the suggestion to change from ISC to MIT without specific context may not be necessary. | 5 | |
Security |
Remove unsupported and problematic
___
**Remove the | 10 |
Maintainability |
Rename the sentinel object class to improve readability and correct the spelling___ **Consider using a more concise and descriptive name for the sentinel object class toimprove readability and maintainability. The name SentinalObject is a misspelling and does not convey its purpose clearly.** [python/composio/client/enums/_action.py [8-10]](https://github.com/ComposioHQ/composio/pull/366/files#diff-a375ebeaef8787ee1ac6fb7a8d50350b8de2041ebe2460dc05aa3de345108624R8-R10) ```diff -class SentinalObject: - """Sentinal object.""" - sentinal = None +class Sentinel: + """Class used as a sentinel value.""" + value = None ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion corrects a spelling mistake and improves readability by providing a more descriptive class name. | 9 |
Remove the deprecated package to clean up dependencies___ **Consider removing the deprecated package@types/dotenv@8.2.0 since dotenv provides its own type definitions. This will clean up the dependencies and reduce potential conflicts or outdated type information.** [js/examples/swe/pnpm-lock.yaml [78-80]](https://github.com/ComposioHQ/composio/pull/366/files#diff-a66e8ef03a9c6f4490cf8935b382b4b27efbad5a7810f24774089d34b81cd0ecR78-R80) ```diff -'@types/dotenv@8.2.0': - resolution: {integrity: sha512-ylSC9GhfRH7m1EUXBXofhgx4lUWmFeQDINW5oLuS+gxWdfUeW4zJdeVTYVkexEW+e2VUvlZR2kGnGGipAWR7kw==} - deprecated: This is a stub types definition. dotenv provides its own type definitions, so you do not need this installed. +# '@types/dotenv@8.2.0' removed as it is deprecated and unnecessary. ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Removing deprecated packages is crucial for maintainability and avoiding potential conflicts. The suggestion correctly identifies and addresses this issue. | 9 | |
Group related actions into subclasses or separate enums for better organization___ **To enhance code organization and readability, consider grouping related actions intosubclasses or separate enums, especially for different services like ASANA , ATTIO , and BREVO .**
[python/composio/client/enums/_action.py [32-34]](https://github.com/ComposioHQ/composio/pull/366/files#diff-a375ebeaef8787ee1ac6fb7a8d50350b8de2041ebe2460dc05aa3de345108624R32-R34)
```diff
-ASANA_CREATE_SUBTASK: 'Action'
-ASANA_CUSTOM_FIELDS_ADDE_NUM_OPTION: 'Action'
-ATTIO_CREATE_AN_OBJECT: 'Action'
-BREVO_ACCOUNT_GET_USER_ACTIVITY_LOGS: 'Action'
+class AsanaActions(Action):
+ CREATE_SUBTASK: Action = None
+ CUSTOM_FIELDS_ADDE_NUM_OPTION: Action = None
+class AttioActions(Action):
+ CREATE_AN_OBJECT: Action = None
+
+class BrevoActions(Action):
+ ACCOUNT_GET_USER_ACTIVITY_LOGS: Action = None
+
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 8Why: Grouping related actions into subclasses enhances code organization and readability, making it easier to manage and understand. | 8 | |
Use a variable for the tool name in the output text for easier maintenance___ **Replace the hardcoded tool name in_output_text with a variable or constant to maintain consistency and ease future updates.** [python/composio/tools/local/shelltool/git_cmds/actions/git_tree.py [26]](https://github.com/ComposioHQ/composio/pull/366/files#diff-d559e1b9aa3fee060072f4a80fef717451b621b14f30e6715009e33a34ef4c26R26-R26) ```diff -_output_text = "Check git_repo_tree.txt for the git-repo-tree results. Use `FILEEDITTOOL_OPEN_FILE` function to check the file." +_output_text = f"Check git_repo_tree.txt for the git-repo-tree results. Use `{FILE_OPEN_FUNCTION}` function to check the file." ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Using a variable for the tool name improves maintainability and consistency, making future updates easier. | 7 | |
Fill in the 'author' field to provide contact or ownership information___ **The 'author' field is empty. It's a good practice to fill in the author field withthe name of the main maintainer or the organization responsible for the package. This can be useful for documentation purposes and potential future queries about the package.** [js/examples/swe/package.json [10]](https://github.com/ComposioHQ/composio/pull/366/files#diff-4b9d91b7b6b142dbd16e62e45931a1c49c1ece7c9df5675383ee499239ce4ee3R10-R10) ```diff -"author": "", +"author": "Your Name or Organization", ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Including the author information is good for maintainability and documentation, but it is not essential for the package's functionality. | 6 | |
Performance |
Add a condition to skip unnecessary iterations when
___
**Add a check to ensure that the | 8 |
Replace the enumeration of
___
**Consider using a more efficient data structure for managing the | 3 | |
Enhancement |
Update the Node.js version requirement for better compatibility and security___ **Consider specifying a more recent minimum Node.js version foransi-styles to align with current LTS versions, enhancing compatibility and security.** [js/examples/swe/pnpm-lock.yaml [117-119]](https://github.com/ComposioHQ/composio/pull/366/files#diff-a66e8ef03a9c6f4490cf8935b382b4b27efbad5a7810f24774089d34b81cd0ecR117-R119) ```diff ansi-styles@5.2.0: resolution: {integrity: sha512-Cxwpt2SfTzTtXcfOlzGEee8O+c+MmUgGrNiBcXnuWxuFJHe6a5Hz7qwhwe5OgaSYI0IJvkLqWX1ASG+cJOkEiA==} - engines: {node: '>=10'} + engines: {node: '>=12'} ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Updating the Node.js version requirement is a good enhancement for compatibility and security, though it is a minor improvement compared to other suggestions. | 7 |
Add a meaningful description to the package.json___ **It is recommended to specify a description for the package to provide clarity on itspurpose and functionality. This can help other developers understand the intent and usage of the package more quickly.** [js/examples/swe/package.json [4]](https://github.com/ComposioHQ/composio/pull/366/files#diff-4b9d91b7b6b142dbd16e62e45931a1c49c1ece7c9df5675383ee499239ce4ee3R4-R4) ```diff -"description": "", +"description": "A simple project demonstrating the integration of LangChain with Node.js", ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Adding a description improves clarity and helps other developers understand the purpose of the package. However, it is not critical for the functionality of the package. | 7 |
**Action:** test (ubuntu-latest, 3.10) |
**Failed stage:** [Unittests](https://github.com/ComposioHQ/composio/actions/runs/10058432830/job/27801572293) [โ] |
**Failed test name:** tests/test_tools/test_local/test_workspace.py::test_workspace |
**Failure summary:**
The action failed because the test tests/test_tools/test_local/test_workspace.py::test_workspace encountered a KeyError . exit_code in the output dictionary.assert output[EXIT_CODE] == 0 failed because EXIT_CODE was not found in the output dictionary. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 504: * [new branch] fix/rag-agent -> origin/fix/rag-agent 505: * [new branch] fix/readme -> origin/fix/readme 506: * [new branch] fix/readme-logo -> origin/fix/readme-logo 507: * [new branch] fix/swe-agent -> origin/fix/swe-agent 508: * [new branch] ft-add-better-help-text -> origin/ft-add-better-help-text 509: * [new branch] ft-apps-id -> origin/ft-apps-id 510: * [new branch] ft-bring-back-core-sdk -> origin/ft-bring-back-core-sdk 511: * [new branch] ft-did-you-mean -> origin/ft-did-you-mean 512: * [new branch] ft-error-tracking -> origin/ft-error-tracking ... 932: tests/test_cli/test_actions.py::TestListActions::test_list_all[arguments3-exptected_outputs3-unexptected_outputs3] PASSED [ 17%] 933: tests/test_cli/test_actions.py::TestListActions::test_tag_not_found PASSED [ 19%] 934: tests/test_cli/test_actions.py::TestListActions::test_limit SKIPPED [ 21%] 935: tests/test_cli/test_actions.py::TestListActions::test_copy PASSED [ 23%] 936: tests/test_cli/test_add.py::TestComposioAdd::test_no_auth PASSED [ 25%] 937: tests/test_cli/test_apps.py::TestList::test_list PASSED [ 27%] 938: tests/test_cli/test_apps.py::TestUpdate::test_update_not_required PASSED [ 29%] 939: tests/test_cli/test_apps.py::TestUpdate::test_update SKIPPED (Needs 940: investigation, this test fails in CI) [ 31%] ... 958: tests/test_client/test_enum.py::test_get_actions PASSED [ 70%] 959: tests/test_storage/test_base.py::test_local_storage PASSED [ 72%] 960: tests/test_tools/test_schema.py::test_openai_schema PASSED [ 74%] 961: tests/test_tools/test_schema.py::test_claude_schema PASSED [ 76%] 962: tests/test_tools/test_toolset.py::test_find_actions_by_tags PASSED [ 78%] 963: tests/test_tools/test_toolset.py::test_uninitialize_app PASSED [ 80%] 964: tests/test_tools/test_local/test_workspace.py::test_outputs PASSED [ 82%] 965: tests/test_tools/test_local/test_workspace.py::test_stderr PASSED [ 85%] 966: tests/test_tools/test_local/test_workspace.py::test_workspace FAILED [ 87%] 967: tests/test_utils/test_decorators.py::test_deprecated PASSED [ 89%] 968: tests/test_utils/test_git.py::test_get_git_user_info PASSED [ 91%] 969: tests/test_utils/test_shared.py::test_get_pydantic_signature_format_from_schema_params PASSED [ 93%] 970: tests/test_utils/test_shared.py::test_json_schema_to_pydantic_field PASSED [ 95%] 971: tests/test_utils/test_shared.py::test_json_schema_to_fields_dict PASSED [ 97%] 972: tests/test_utils/test_url.py::test_get_web_url PASSED [100%] 973: =================================== FAILURES =================================== ... 990: > _check_output( 991: output=toolset.execute_action( 992: action=Action.FILETOOL_GIT_CLONE, 993: params={"repo_name": "angrybayblade/web"}, 994: ) 995: ) 996: tests/test_tools/test_local/test_workspace.py:110: 997: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 998: output = {'current_working_directory': '/home/runner/work/composio/composio/python/web', 'error': '', 'message': "On branch main\nYour branch is up to date with 'origin/main'.\n\nnothing to commit, working tree clean\n", 'success': True} 999: def _check_output(output: dict) -> None: 1000: """Check tool output.""" 1001: > assert output[EXIT_CODE] == 0, f"output: {output}" 1002: E KeyError: 'exit_code' 1003: tests/test_tools/test_local/test_workspace.py:44: KeyError 1004: ------------------------------ Captured log call ------------------------------- 1005: DEBUG workspace:toolset.py:109 Trying to get github access token for self.entity_id='default' 1006: DEBUG workspace:http.py:106 GET ***/v1/connectedAccounts?user_uuid=default&showActiveOnly=true - {} 1007: DEBUG workspace:http.py:106 GET ***/v1/connectedAccounts/a7bd3cfd-3c80-4c80-9ef8-ff7cf1a60507 - {} 1008: DEBUG workspace:toolset.py:119 Using `***` with scopes: admin:org 1009: DEBUG workspace:factory.py:76 Creating workspace with config=Config(composio_api_key='***', composio_base_url='***', github_access_token='***', environment=None, persistent=False, ssh=None) 1010: DEBUG workspace:workspace.py:76 Setting up SSH client for workspace 5SU5Te 1011: DEBUG workspace:workspace.py:92 Setting up SSH client for workspace failed with error: No authentication methods available ... 1020: .tox/unittests/lib/python3.10/site-packages/paramiko/pkey.py:100 1021: /home/runner/work/composio/composio/python/.tox/unittests/lib/python3.10/site-packages/paramiko/pkey.py:100: CryptographyDeprecationWarning: TripleDES has been moved to cryptography.hazmat.decrepit.ciphers.algorithms.TripleDES and will be removed from this module in 48.0.0. 1022: "cipher": algorithms.TripleDES, 1023: .tox/unittests/lib/python3.10/site-packages/paramiko/transport.py:259 1024: /home/runner/work/composio/composio/python/.tox/unittests/lib/python3.10/site-packages/paramiko/transport.py:259: CryptographyDeprecationWarning: TripleDES has been moved to cryptography.hazmat.decrepit.ciphers.algorithms.TripleDES and will be removed from this module in 48.0.0. 1025: "class": algorithms.TripleDES, 1026: .tox/unittests/lib/python3.10/site-packages/pydantic/_internal/_config.py:291 1027: .tox/unittests/lib/python3.10/site-packages/pydantic/_internal/_config.py:291 1028: /home/runner/work/composio/composio/python/.tox/unittests/lib/python3.10/site-packages/pydantic/_internal/_config.py:291: PydanticDeprecatedSince20: Support for class-based `config` is deprecated, use ConfigDict instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.8/migration/ ... 1238: examples/Podcast_summarizer_Agents/Tools/composio_slack.py 4 4 0% 1-5 1239: examples/Podcast_summarizer_Agents/__init__.py 0 0 100% 1240: examples/Podcast_summarizer_Agents/main.py 15 15 0% 1-21 1241: -------------------------------------------------------------------------------------------------------------- 1242: TOTAL 10788 2814 74% 1243: Coverage HTML written to dir htmlcov 1244: Coverage XML written to file coverage.xml 1245: =========================== short test summary info ============================ 1246: FAILED tests/test_tools/test_local/test_workspace.py::test_workspace - KeyError: 'exit_code' 1247: ============= 1 failed, 41 passed, 5 skipped, 5 warnings in 36.60s ============= 1248: unittests: exit 1 (37.49 seconds) /home/runner/work/composio/composio/python> pytest -vvv -rfE --doctest-modules composio/ tests/ --cov=composio --cov=examples --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc pid=5603 1249: .pkg: _exit> python /opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__ 1250: unittests: FAIL code 1 (62.90=setup[20.85]+cmd[4.56,37.49] seconds) 1251: evaluation failed :( (63.13 seconds) 1252: ##[error]Process completed with exit code 1. ``` |
PR Type
Enhancement, Tests
Description
App
class.GitRepoTree
to reference a specific function.shell_id
field inShellRequest
.process
is defined before adding an exit handler.Changes walkthrough ๐
1 files
_app.py
Standardize string quotes and formatting in `App` class.
python/composio/client/enums/_app.py
App
class attributes.6 files
git_tree.py
Update output text to reference specific function.
python/composio/tools/local/shelltool/git_cmds/actions/git_tree.py - Updated output text to use a specific function name.
prompts.ts
Add software engineer example prompt script.
js/examples/swe/src/prompts.ts - Added a new prompt script for a software engineer example.
inputs.ts
Add input handling and validation functions for GitHub data.
js/examples/swe/src/agents/inputs.ts
and issue data.
agent.ts
Initialize software engineer agent with tools.
js/examples/swe/src/agents/agent.ts
app.ts
Add main application script for software engineer agent.
js/examples/swe/src/app.ts - Added main application script to run the software engineer agent.
base.toolset.ts
Add check for `process` definition before adding exit handler.
js/src/sdk/base.toolset.ts
process
is defined before adding an exithandler.
1 files
exec.py
Clarify optional nature of `shell_id` field in `ShellRequest`.
python/composio/tools/local/shelltool/shell_exec/actions/exec.py - Added clarification to the description of `shell_id` field.
1 files
pnpm-lock.yaml
Add lockfile for software engineer example dependencies.
js/examples/swe/pnpm-lock.yaml - Added lockfile for dependencies of the software engineer example.
1 files
package.json
Add package configuration for software engineer example.
js/examples/swe/package.json - Added package configuration for the software engineer example.
1 files
package.json
Bump version to 0.1.8.
js/package.json - Bumped version from 0.1.7 to 0.1.8.
1 files
_action.py
...
python/composio/client/enums/_action.py ...