search

ComposioHQ / composio

Composio equips agents with well-crafted tools empowering them to tackle complex tasks
https://docs.composio.dev
Other
1.3k stars 425 forks source link

Avoid Composio API Key in SWE #230

Closed kaavee315 closed 1 week ago

kaavee315 commented 1 week ago

User description

Avoid using Composio API Key in SWE

PR Type

Enhancement, Bug fix

Description

Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Refactor Composio client to use properties for API key and HTTP client
python/composio/client/__init__.py
  • Introduced properties for api_key and http with validation and
    environment variable support.
  • Removed direct initialization of HttpClient and api_key in the
    constructor.
  • Enhanced error handling for missing API key.
    		•  +40/-12 
    base_swe_agent.py
    Add logging and error handling in BaseSWEAgent                     
    python/swe/composio_swe/agent/base_swe_agent.py
  • Added logging functionality to BaseSWEAgent.
  • Enhanced error handling for action execution response.
  • Replaced print statements with logger info calls.
    		•  +8/-4     

    💡 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 week ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Error Handling:
    Ensure that the error handling logic in Composio class is robust, especially around API key validation and HTTP client initialization.
    Parameter Change:
    The change from base_commit to commit_id in the execute method call within BaseSWEAgent needs verification to ensure it aligns with expected API parameters.
    Logging Integration:
    Verify that the new logging integration in BaseSWEAgent correctly handles all scenarios and that the log outputs are as expected.
    codiumai-pr-agent-pro[bot] commented 1 week ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add exception handling around the API key validation to manage failures gracefully ___ **Consider using a more robust approach for API key validation by handling potential
    exceptions that might be thrown by validate_api_key method, such as network errors or
    invalid responses.** [python/composio/client/__init__.py [54-57]](https://github.com/ComposioHQ/composio/pull/230/files#diff-5e245b6db29b5a3be6b817cd96ca8befe7dab1fdbeb29d4ff98e871e7590b665R54-R57) ```diff -self._api_key = self.validate_api_key( - key=t.cast(str, self._api_key), - base_url=self.base_url, -) +try: + self._api_key = self.validate_api_key( + key=t.cast(str, self._api_key), + base_url=self.base_url, + ) +except Exception as e: + logging.error(f"Failed to validate API key: {str(e)}") + raise ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Adding exception handling around the API key validation is crucial for managing potential failures, such as network errors or invalid responses, thus improving the reliability of the code.
    		10
    Possible bug
    ✅ Remove the return statement after raising an exception in the API key getter ___
    Suggestion Impact:The commit removed the return statement after raising an exception in the api_key property getter, aligning with the suggestion to avoid returning an empty string. code diff: ```diff if self._api_key is None: - raise_api_key_missing() - return "" - if not self._api_key_validated: - self._api_key = self.validate_api_key( - key=t.cast(str, self._api_key), - base_url=self.base_url, - ) - self._api_key_validated = True + raise ApiKeyNotProvidedError() ```
    ___ **Avoid returning an empty string after raising an exception in the api_key property getter.
    This could lead to further errors downstream where a valid API key is expected.** [python/composio/client/__init__.py [50-52]](https://github.com/ComposioHQ/composio/pull/230/files#diff-5e245b6db29b5a3be6b817cd96ca8befe7dab1fdbeb29d4ff98e871e7590b665R50-R52) ```diff if self._api_key is None: raise_api_key_missing() - return "" ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Returning an empty string after raising an exception can lead to unexpected behavior and further errors. Removing the return statement ensures that the exception is properly handled.
    		9
    Possible issue
    Add error handling for missing environment variable for API key ___ **Consider handling the case where the environment variable ENV_COMPOSIO_API_KEY is not set.
    Currently, if the environment variable is not set, the code will raise an exception
    without a clear message related to the missing API key.** [python/composio/client/__init__.py [47-49]](https://github.com/ComposioHQ/composio/pull/230/files#diff-5e245b6db29b5a3be6b817cd96ca8befe7dab1fdbeb29d4ff98e871e7590b665R47-R49) ```diff env_api_key = os.environ.get(ENV_COMPOSIO_API_KEY) if env_api_key: self._api_key = env_api_key +else: + raise ValueError("ENV_COMPOSIO_API_KEY environment variable is not set.") ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding error handling for a missing environment variable improves robustness and provides clearer error messages, which is important for debugging and user experience.
    		8
    Best practice
    Use logging instead of print for outputting the git clone completion time ___ **Replace the print statement with a logging statement for consistency and better control
    over log management.** [python/swe/composio_swe/agent/base_swe_agent.py [68]](https://github.com/ComposioHQ/composio/pull/230/files#diff-501c62b63767c54587e8c200b489666e4cbd2bb9b0605bf95e67c39b89c3bc8aR68-R68) ```diff -print("git clone completed, time taken: %s", git_clone_time) +self.logger.info("git clone completed, time taken: %s", git_clone_time) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Replacing print statements with logging is a best practice for better log management and consistency, although it is a minor improvement.
    		7
    codiumai-pr-agent-pro[bot] commented 1 week ago

    CI Failure Feedback 🧐

    (Checks updated until commit https://github.com/ComposioHQ/composio/commit/949638b84c50ecc8678734977ae59ae81f3b189a)

    **Action:** test (ubuntu-latest, 3.9)
    **Failed stage:** [Unittests](https://github.com/ComposioHQ/composio/actions/runs/9712288263/job/26806771733) [❌]
    **Failed test name:** test_raise_invalid_api_key
    **Failure summary:** The action failed because the test test_raise_invalid_api_key did not raise the expected exception.
  • The test expected a ComposioClientError to be raised when an invalid API key is used.
  • The test is located in the file tests/test_client/test_client.py at line 14.
    • Relevant error logs: ```yaml 1: ##[group]Operating System 2: Ubuntu ... 496: * [new branch] feat/opensource-ready -> origin/feat/opensource-ready 497: * [new branch] feat/slack-assistant -> origin/feat/slack-assistant 498: * [new branch] fix/readme -> origin/fix/readme 499: * [new branch] fix/readme-logo -> origin/fix/readme-logo 500: * [new branch] ft-add-better-help-text -> origin/ft-add-better-help-text 501: * [new branch] ft-apps-id -> origin/ft-apps-id 502: * [new branch] ft-bring-back-core-sdk -> origin/ft-bring-back-core-sdk 503: * [new branch] ft-did-you-mean -> origin/ft-did-you-mean 504: * [new branch] ft-error-tracking -> origin/ft-error-tracking ... 905: tests/test_cli/test_actions.py::TestListActions::test_list_all[arguments3-exptected_outputs3-unexptected_outputs3] PASSED [ 22%] 906: tests/test_cli/test_actions.py::TestListActions::test_tag_not_found PASSED [ 24%] 907: tests/test_cli/test_actions.py::TestListActions::test_limit SKIPPED [ 26%] 908: tests/test_cli/test_actions.py::TestListActions::test_copy PASSED [ 28%] 909: tests/test_cli/test_add.py::TestComposioAdd::test_no_auth PASSED [ 31%] 910: tests/test_cli/test_apps.py::TestList::test_list PASSED [ 33%] 911: tests/test_cli/test_apps.py::TestUpdate::test_update_not_required PASSED [ 35%] 912: tests/test_cli/test_apps.py::TestUpdate::test_update SKIPPED (Needs 913: investigation, this test fails in CI) [ 37%] 914: tests/test_cli/test_connections.py::TestListConnections::test_list_all PASSED [ 40%] 915: tests/test_cli/test_connections.py::TestListConnections::test_list_one PASSED [ 42%] 916: tests/test_cli/test_context.py::test_login_required_decorator PASSED [ 44%] 917: tests/test_cli/test_integrations.py::TestIntegration::test_list PASSED [ 46%] 918: tests/test_cli/test_login.py::TestLogin::test_user_already_logged_in PASSED [ 48%] 919: tests/test_client/test_base.py::test_raise_if_required PASSED [ 51%] 920: tests/test_client/test_base.py::test_invalid_data_object PASSED [ 53%] 921: tests/test_client/test_client.py::test_raise_invalid_api_key FAILED [ 55%] ... 934: tests/test_tools/test_schema.py::test_claude_schema PASSED [ 84%] 935: tests/test_tools/test_toolset.py::test_find_actions_by_tags PASSED [ 86%] 936: tests/test_utils/test_decorators.py::test_deprecated PASSED [ 88%] 937: tests/test_utils/test_git.py::test_get_git_user_info PASSED [ 91%] 938: tests/test_utils/test_shared.py::test_get_pydantic_signature_format_from_schema_params PASSED [ 93%] 939: tests/test_utils/test_shared.py::test_json_schema_to_pydantic_field PASSED [ 95%] 940: tests/test_utils/test_shared.py::test_json_schema_to_fields_dict PASSED [ 97%] 941: tests/test_utils/test_url.py::test_get_web_url PASSED [100%] 942: =================================== FAILURES =================================== 943: __________________________ test_raise_invalid_api_key __________________________ 944: def test_raise_invalid_api_key() -> None: 945: """Test invalid API key.""" 946: with pytest.raises(ComposioClientError, match="API Key is not valid!"): 947: > Composio(api_key="API_KEY") 948: E Failed: DID NOT RAISE 949: tests/test_client/test_client.py:14: Failed 950: =============================== warnings summary =============================== 951: composio/client/http.py:18 952: /home/runner/work/composio/composio/python/composio/client/http.py:18: DeprecationWarning: Inheritance class AsyncHttpClient from ClientSession is discouraged 953: class AsyncHttpClient(AsyncSession, logging.WithLogger): 954: .tox/unittests/lib/python3.9/site-packages/pydantic/_internal/_config.py:284 955: /home/runner/work/composio/composio/python/.tox/unittests/lib/python3.9/site-packages/pydantic/_internal/_config.py:284: 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.7/migration/ ... 1076: composio/utils/shared.py 117 83 29% 44, 47-51, 54-58, 61-77, 83, 101-104, 153-158, 174-221, 247-292 1077: composio/utils/url.py 10 1 90% 35 1078: examples/crewai_ci_chart.py 14 14 0% 1-38 1079: -------------------------------------------------------------------------------------------------------------------- 1080: TOTAL 8318 1643 80% 1081: Coverage HTML written to dir htmlcov 1082: Coverage XML written to file coverage.xml 1083: =========================== short test summary info ============================ 1084: FAILED tests/test_client/test_client.py::test_raise_invalid_api_key - Failed: DID NOT RAISE 1085: ============= 1 failed, 37 passed, 7 skipped, 2 warnings in 20.92s ============= 1086: unittests: exit 1 (21.81 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=5545 1087: .pkg: _exit> python /opt/hostedtoolcache/Python/3.9.19/x64/lib/python3.9/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__ 1088: unittests: FAIL code 1 (46.50=setup[18.57]+cmd[6.13,21.81] seconds) 1089: evaluation failed :( (46.65 seconds) 1090: ##[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/).