ComposioHQ / composio

Composio equip's your AI agents & LLMs with 100+ high-quality integrations via function calling
https://docs.composio.dev
Other
7.47k stars 2.33k forks source link

fix file manager and agent templates #363

Closed sohamganatra closed 1 month ago

sohamganatra commented 1 month ago

PR Type

Enhancement, Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
14 files
manager.py
Enhance FileManager with error handling and command execution

python/composio/tools/env/filemanager/manager.py
  • Added error handling for chdir method.
  • Introduced current_dir and execute_command methods.
  • +42/-3   
    __init__.py
    Add imports for Git-related actions                                           

    python/composio/tools/local/filetool/actions/__init__.py - Added imports for new Git-related actions.
    +3/-0     
    git_clone.py
    Introduce GitClone action for repository cloning                 

    python/composio/tools/local/filetool/actions/git_clone.py
  • Introduced GitClone action for cloning Git repositories.
  • Added request and response schemas for GitClone.
  • +130/-0 
    git_patch.py
    Introduce GitPatch action for generating patches                 

    python/composio/tools/local/filetool/actions/git_patch.py
  • Introduced GitPatch action for generating Git patches.
  • Added request and response schemas for GitPatch.
  • +84/-0   
    git_repo_tree.py
    Introduce GitRepoTree action for creating repository tree

    python/composio/tools/local/filetool/actions/git_repo_tree.py
  • Introduced GitRepoTree action for creating repository tree.
  • Added request and response schemas for GitRepoTree.
  • +102/-0 
    tool.py
    Register new Git-related actions in FileTool                         

    python/composio/tools/local/filetool/tool.py - Registered new Git-related actions in `FileTool`.
    +6/-0     
    agent.py
    Update required tools for crewai_agent                                     

    python/swe/examples/crewai_agent/agent.py - Removed `App.GITCMDTOOL` and added `App.SEARCHTOOL`.
    +1/-2     
    main.py
    Replace GITCMDTOOL_GET_PATCH_CMD with FILETOOL_GIT_PATCH 

    python/swe/examples/crewai_agent/main.py
  • Replaced Action.GITCMDTOOL_GET_PATCH_CMD with
    Action.FILETOOL_GIT_PATCH.
  • +1/-1     
    base.py
    Replace GITCMDTOOL_GET_PATCH_CMD with FILETOOL_GIT_PATCH 

    python/swe/swekit/agents/base.py
  • Replaced Action.GITCMDTOOL_GET_PATCH_CMD with
    Action.FILETOOL_GIT_PATCH.
  • +1/-1     
    run_evaluation.py
    Replace GITCMDTOOL_GET_PATCH_CMD with FILETOOL_GIT_PATCH 

    python/swe/swekit/benchmark/run_evaluation.py
  • Replaced Action.GITCMDTOOL_GET_PATCH_CMD with
    Action.FILETOOL_GIT_PATCH.
  • +1/-1     
    utils.py
    Replace GITCMDTOOL_GITHUB_CLONE_CMD with FILETOOL_GIT_CLONE

    python/swe/swekit/benchmark/utils.py
  • Replaced Action.GITCMDTOOL_GITHUB_CLONE_CMD with
    Action.FILETOOL_GIT_CLONE.
  • +9/-9     
    test_workspace.py
    Replace GITCMDTOOL actions with FILETOOL actions in tests

    python/tests/test_tools/test_local/test_workspace.py
  • Replaced Action.GITCMDTOOL_GITHUB_CLONE_CMD with
    Action.FILETOOL_GIT_CLONE.
  • Replaced Action.GITCMDTOOL_GET_PATCH_CMD with
    Action.FILETOOL_GIT_PATCH.
  • +2/-2     
    agent.template
    Update required tools in agent template                                   

    python/swe/swekit/scaffold/templates/crewai/agent.template - Removed `App.GITCMDTOOL` and `App.HISTORYFETCHERTOOL`.
    +0/-3     
    main.template
    Replace GITCMDTOOL_GET_PATCH_CMD with FILETOOL_GIT_PATCH 

    python/swe/swekit/scaffold/templates/crewai/main.template
  • Replaced Action.GITCMDTOOL_GET_PATCH_CMD with
    Action.FILETOOL_GIT_PATCH.
  • +1/-1     
    Bug fix
    2 files
    open.py
    Add error handling for empty file content                               

    python/composio/tools/local/filetool/actions/open.py - Added error handling for empty file content in `OpenFileResponse`.
    +4/-1     
    inputs.template
    Fix typo in user input prompt                                                       

    python/swe/swekit/scaffold/templates/crewai/inputs.template - Fixed typo in user input prompt.
    +1/-1     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 4 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ต๐Ÿ”ตโšช
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ Security concerns

    Yes - Shell Injection:
    The use of `subprocess.run` with `shell=True` in `execute_command` method of `FileManager` can lead to shell injection vulnerabilities. - Command Injection: The `git_clone_cmd` function in `git_clone.py` constructs shell commands using potentially unsanitized input, which may lead to command injection vulnerabilities.
    โšก Key issues to review

    Security Concern
    The method `execute_command` uses `subprocess.run` with `shell=True`, which can lead to shell injection vulnerabilities if the command is constructed from user inputs or external sources. Security Concern
    The method `git_clone_cmd` constructs a command with potential user input (`repo`) and executes it, which might lead to command injection if not properly sanitized. Exception Handling
    The method `execute_on_file_manager` in `GitClone` action raises a generic `Exception` which could obscure the underlying error, making debugging more difficult. Error Handling
    The `chdir` method raises a generic `Exception` for `OSError`, which could be more specific to aid in error handling and debugging.
    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Security
    โœ… Improve security by avoiding the use of shell=True in subprocess calls ___
    Suggestion Impact:The commit modified the `execute_command` method to change the subprocess call, including removing `shell=True`, which aligns with the suggestion to avoid potential security risks. code diff: ```diff - def execute_command(self, command: str) -> str: + def execute_command(self, command: str) -> t.Tuple[str, t.Optional[str]]: """Execute a command in the current working directory.""" try: result = subprocess.run( @@ -333,11 +345,11 @@ check=True, capture_output=True, text=True, - timeout=120, + timeout=360, cwd=self.working_dir ) - return result.stdout + return result.stdout, None except subprocess.CalledProcessError as e: - return f"Error executing command: {e.stderr}" + return "", f"Error executing command: {e.stderr}" except subprocess.TimeoutExpired: - return "Command execution timed out after 120 seconds"+ return "", "TIMEOUT: Command execution timed out after 120 seconds" ```
    ___ **Replace the use of subprocess.run with a more secure method that does not use
    shell=True to avoid potential security risks such as shell injection.** [python/composio/tools/env/filemanager/manager.py [330-337]](https://github.com/ComposioHQ/composio/pull/363/files#diff-56840966f6768a0dd21d9e8fba33cf59915a3ac5331cac021bfd0490d875b481R330-R337) ```diff result = subprocess.run( - command, - shell=True, + command.split(), check=True, capture_output=True, text=True, timeout=120, cwd=self.working_dir ) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: This suggestion addresses a significant security concern by avoiding the use of `shell=True`, which can lead to shell injection vulnerabilities.
    10
    Maintainability
    Use specific exceptions for better error handling and clarity ___ **Handle specific exceptions for FileNotFoundError and PermissionError separately to
    provide more accurate error handling and messages.** [python/composio/tools/env/filemanager/manager.py [98]](https://github.com/ComposioHQ/composio/pull/363/files#diff-56840966f6768a0dd21d9e8fba33cf59915a3ac5331cac021bfd0490d875b481R98-R98) ```diff -except OSError as e: - raise Exception(f"OS error: {str(e)}") +except FileNotFoundError as fnf_error: + raise FileNotFoundError(f"Directory not found: {str(fnf_error)}") +except PermissionError as perm_error: + raise PermissionError(f"Permission denied: {str(perm_error)}") +except OSError as os_error: + raise Exception(f"OS error: {str(os_error)}") ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Handling specific exceptions like `FileNotFoundError` and `PermissionError` separately improves error clarity and maintainability.
    8
    Refactor permission checks into a separate method for better code organization ___ **Refactor the chdir method to separate concerns by moving the permission check into a
    separate method, improving code readability and maintainability.** [python/composio/tools/env/filemanager/manager.py [94-95]](https://github.com/ComposioHQ/composio/pull/363/files#diff-56840966f6768a0dd21d9e8fba33cf59915a3ac5331cac021bfd0490d875b481R94-R95) ```diff -if not os.access(new_dir, os.R_OK | os.X_OK): - raise PermissionError(f"Permission denied: Cannot access '{new_dir}'") +def check_directory_permissions(directory): + if not os.access(directory, os.R_OK | os.X_OK): + raise PermissionError(f"Permission denied: Cannot access '{directory}'") +check_directory_permissions(new_dir) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Refactoring the permission check into a separate method improves code readability and maintainability, but the impact is relatively minor.
    6
    Best practice
    Use specific exception types for clearer error handling ___ **Replace the generic Exception with more specific exceptions to provide clearer error
    messaging and handling.** [python/composio/tools/env/filemanager/manager.py [98]](https://github.com/ComposioHQ/composio/pull/363/files#diff-56840966f6768a0dd21d9e8fba33cf59915a3ac5331cac021bfd0490d875b481R98-R98) ```diff except OSError as e: - raise Exception(f"OS error: {str(e)}") + raise OSError(f"OS error: {str(e)}") ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Using more specific exceptions instead of a generic `Exception` improves error handling clarity, but the improvement is minor compared to the previous suggestion.
    7
    codiumai-pr-agent-pro[bot] commented 1 month ago

    CI Failure Feedback ๐Ÿง

    (Checks updated until commit https://github.com/ComposioHQ/composio/commit/50ac210d416635ec1f1cc804ae6a1710993fe6d4)

    **Action:** test (ubuntu-latest, 3.10)
    **Failed stage:** [Unittests](https://github.com/ComposioHQ/composio/actions/runs/10057602391/job/27798956634) [โŒ]
    **Failed test name:** test_workspace
    **Failure summary:** The action failed because the test test_workspace in the file
    tests/test_tools/test_local/test_workspace.py encountered a KeyError.
  • The error occurred because the key exit_code was not found in the output dictionary.
  • The assertion assert output[EXIT_CODE] == 0 failed due to the missing exit_code key.
  • Relevant error logs: ```yaml 1: ##[group]Operating System 2: Ubuntu ... 505: * [new branch] fix/rag-agent -> origin/fix/rag-agent 506: * [new branch] fix/readme -> origin/fix/readme 507: * [new branch] fix/readme-logo -> origin/fix/readme-logo 508: * [new branch] fix/swe-agent -> origin/fix/swe-agent 509: * [new branch] ft-add-better-help-text -> origin/ft-add-better-help-text 510: * [new branch] ft-apps-id -> origin/ft-apps-id 511: * [new branch] ft-bring-back-core-sdk -> origin/ft-bring-back-core-sdk 512: * [new branch] ft-did-you-mean -> origin/ft-did-you-mean 513: * [new branch] ft-error-tracking -> origin/ft-error-tracking ... 934: tests/test_cli/test_actions.py::TestListActions::test_list_all[arguments3-exptected_outputs3-unexptected_outputs3] PASSED [ 17%] 935: tests/test_cli/test_actions.py::TestListActions::test_tag_not_found PASSED [ 19%] 936: tests/test_cli/test_actions.py::TestListActions::test_limit SKIPPED [ 21%] 937: tests/test_cli/test_actions.py::TestListActions::test_copy PASSED [ 23%] 938: tests/test_cli/test_add.py::TestComposioAdd::test_no_auth PASSED [ 25%] 939: tests/test_cli/test_apps.py::TestList::test_list PASSED [ 27%] 940: tests/test_cli/test_apps.py::TestUpdate::test_update_not_required PASSED [ 29%] 941: tests/test_cli/test_apps.py::TestUpdate::test_update SKIPPED (Needs 942: investigation, this test fails in CI) [ 31%] ... 960: tests/test_client/test_enum.py::test_get_actions PASSED [ 70%] 961: tests/test_storage/test_base.py::test_local_storage PASSED [ 72%] 962: tests/test_tools/test_schema.py::test_openai_schema PASSED [ 74%] 963: tests/test_tools/test_schema.py::test_claude_schema PASSED [ 76%] 964: tests/test_tools/test_toolset.py::test_find_actions_by_tags PASSED [ 78%] 965: tests/test_tools/test_toolset.py::test_uninitialize_app PASSED [ 80%] 966: tests/test_tools/test_local/test_workspace.py::test_outputs PASSED [ 82%] 967: tests/test_tools/test_local/test_workspace.py::test_stderr PASSED [ 85%] 968: tests/test_tools/test_local/test_workspace.py::test_workspace FAILED [ 87%] 969: tests/test_utils/test_decorators.py::test_deprecated PASSED [ 89%] 970: tests/test_utils/test_git.py::test_get_git_user_info PASSED [ 91%] 971: tests/test_utils/test_shared.py::test_get_pydantic_signature_format_from_schema_params PASSED [ 93%] 972: tests/test_utils/test_shared.py::test_json_schema_to_pydantic_field PASSED [ 95%] 973: tests/test_utils/test_shared.py::test_json_schema_to_fields_dict PASSED [ 97%] 974: tests/test_utils/test_url.py::test_get_web_url PASSED [100%] 975: =================================== FAILURES =================================== ... 992: > _check_output( 993: output=toolset.execute_action( 994: action=Action.FILETOOL_GIT_CLONE, 995: params={"repo_name": "angrybayblade/web"}, 996: ) 997: ) 998: tests/test_tools/test_local/test_workspace.py:110: 999: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 1000: 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} 1001: def _check_output(output: dict) -> None: 1002: """Check tool output.""" 1003: > assert output[EXIT_CODE] == 0, f"output: {output}" 1004: E KeyError: 'exit_code' 1005: tests/test_tools/test_local/test_workspace.py:44: KeyError 1006: ------------------------------ Captured log call ------------------------------- 1007: DEBUG workspace:toolset.py:109 Trying to get github access token for self.entity_id='default' 1008: DEBUG workspace:http.py:106 GET ***/v1/connectedAccounts?user_uuid=default&showActiveOnly=true - {} 1009: DEBUG workspace:http.py:106 GET ***/v1/connectedAccounts/a7bd3cfd-3c80-4c80-9ef8-ff7cf1a60507 - {} 1010: DEBUG workspace:toolset.py:119 Using `***` with scopes: admin:org 1011: 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) 1012: DEBUG workspace:workspace.py:76 Setting up SSH client for workspace DosYeG 1013: DEBUG workspace:workspace.py:92 Setting up SSH client for workspace failed with error: No authentication methods available ... 1022: .tox/unittests/lib/python3.10/site-packages/paramiko/pkey.py:100 1023: /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. 1024: "cipher": algorithms.TripleDES, 1025: .tox/unittests/lib/python3.10/site-packages/paramiko/transport.py:259 1026: /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. 1027: "class": algorithms.TripleDES, 1028: .tox/unittests/lib/python3.10/site-packages/pydantic/_internal/_config.py:291 1029: .tox/unittests/lib/python3.10/site-packages/pydantic/_internal/_config.py:291 1030: /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/ ... 1240: examples/Podcast_summarizer_Agents/Tools/composio_slack.py 4 4 0% 1-5 1241: examples/Podcast_summarizer_Agents/__init__.py 0 0 100% 1242: examples/Podcast_summarizer_Agents/main.py 15 15 0% 1-21 1243: -------------------------------------------------------------------------------------------------------------- 1244: TOTAL 10788 2814 74% 1245: Coverage HTML written to dir htmlcov 1246: Coverage XML written to file coverage.xml 1247: =========================== short test summary info ============================ 1248: FAILED tests/test_tools/test_local/test_workspace.py::test_workspace - KeyError: 'exit_code' 1249: ============= 1 failed, 41 passed, 5 skipped, 5 warnings in 39.21s ============= 1250: unittests: exit 1 (40.15 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=5615 1251: .pkg: _exit> python /opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__ 1252: unittests: FAIL code 1 (63.80=setup[18.93]+cmd[4.73,40.15] seconds) 1253: evaluation failed :( (63.95 seconds) 1254: ##[error]Process completed with exit code 1. ```

    โœจ CI feedback usage guide:
    The CI feedback tool (`/checks)` automatically triggers when a PR has a failed check. The tool analyzes the failed checks and provides several feedbacks: - Failed stage - Failed test name - Failure summary - Relevant error logs In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: ``` /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}" ``` where `{repo_name}` is the name of the repository, `{run_number}` is the run number of the failed check, and `{job_number}` is the job number of the failed check. #### Configuration options - `enable_auto_checks_feedback` - if set to true, the tool will automatically provide feedback when a check is failed. Default is true. - `excluded_checks_list` - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list. - `enable_help_text` - if set to true, the tool will provide a help message with the feedback. Default is true. - `persistent_comment` - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true. - `final_update_message` - if `persistent_comment` is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true. See more information about the `checks` tool in the [docs](https://pr-agent-docs.codium.ai/tools/ci_feedback/).