From 06a5689e8a29ddd581f7fdf962c0df98f850622a Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 9 Jun 2025 14:46:50 +0000 Subject: [PATCH] Implement AI code review suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Enhanced SSL function documentation with detailed examples and environment variable precedence - Added CA bundle file format validation (.pem, .crt, .cer) with warnings for unsupported formats - Improved error handling with structured solutions and current CA bundle path display - Added comprehensive tests for file format validation and warning scenarios - Enhanced user guidance for SSL certificate configuration issues Addresses feedback from joaomdmoura's AI code review for better documentation, error handling, and path validation as requested. Co-Authored-By: João --- src/crewai/cli/provider.py | 20 +++++++++++++++++--- tests/cli/test_provider_ssl.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/crewai/cli/provider.py b/src/crewai/cli/provider.py index e9e18638c..a7873a5b4 100644 --- a/src/crewai/cli/provider.py +++ b/src/crewai/cli/provider.py @@ -170,11 +170,22 @@ def get_ssl_verify_config(): Example: >>> get_ssl_verify_config() '/path/to/ca-bundle.pem' + + >>> os.environ['REQUESTS_CA_BUNDLE'] = '/custom/ca-bundle.pem' + >>> get_ssl_verify_config() + '/custom/ca-bundle.pem' """ for env_var in ['REQUESTS_CA_BUNDLE', 'SSL_CERT_FILE', 'CURL_CA_BUNDLE']: ca_bundle = os.environ.get(env_var) - if ca_bundle and os.path.isfile(ca_bundle): - return ca_bundle + if ca_bundle: + ca_path = Path(ca_bundle) + if ca_path.is_file() and ca_path.suffix in ['.pem', '.crt', '.cer']: + return str(ca_path) + elif ca_path.is_file(): + click.secho(f"Warning: CA bundle file {ca_bundle} may not be in expected format (.pem, .crt, .cer)", fg="yellow") + return str(ca_path) + else: + click.secho(f"Warning: CA bundle path {ca_bundle} from {env_var} does not exist", fg="yellow") return certifi.where() @@ -200,7 +211,10 @@ def fetch_provider_data(cache_file): except requests.exceptions.SSLError as e: click.secho(f"SSL certificate verification failed: {e}", fg="red") click.secho(f"Current CA bundle path: {ssl_config}", fg="yellow") - click.secho("Try setting REQUESTS_CA_BUNDLE environment variable to your CA bundle path", fg="yellow") + click.secho("Solutions:", fg="cyan") + click.secho(" 1. Set REQUESTS_CA_BUNDLE environment variable to your CA bundle path", fg="yellow") + click.secho(" 2. Ensure your CA bundle file is in .pem, .crt, or .cer format", fg="yellow") + click.secho(" 3. Contact your system administrator for the correct CA bundle", fg="yellow") return None except requests.RequestException as e: click.secho(f"Error fetching provider data: {e}", fg="red") diff --git a/tests/cli/test_provider_ssl.py b/tests/cli/test_provider_ssl.py index bd99697df..0ed2e61ed 100644 --- a/tests/cli/test_provider_ssl.py +++ b/tests/cli/test_provider_ssl.py @@ -71,6 +71,35 @@ class TestSSLConfiguration: result = get_ssl_verify_config() assert result == '/path/to/certifi/cacert.pem' + def test_get_ssl_verify_config_file_format_validation(self): + """Test that CA bundle file format validation works correctly.""" + with tempfile.NamedTemporaryFile(suffix=".pem", delete=False) as temp_file: + temp_path = temp_file.name + + try: + with patch.dict(os.environ, {"REQUESTS_CA_BUNDLE": temp_path}): + result = get_ssl_verify_config() + assert result == temp_path + finally: + os.unlink(temp_path) + + def test_get_ssl_verify_config_unsupported_format_warning(self): + """Test that unsupported file formats still work but show warning.""" + with tempfile.NamedTemporaryFile(suffix=".txt", delete=False) as temp_file: + temp_path = temp_file.name + + try: + with patch.dict(os.environ, {"REQUESTS_CA_BUNDLE": temp_path}): + with patch('click.secho') as mock_secho: + result = get_ssl_verify_config() + assert result == temp_path + mock_secho.assert_called_with( + f"Warning: CA bundle file {temp_path} may not be in expected format (.pem, .crt, .cer)", + fg="yellow" + ) + finally: + os.unlink(temp_path) + class TestFetchProviderDataSSL: def test_fetch_provider_data_uses_ssl_config(self): @@ -99,7 +128,8 @@ class TestFetchProviderDataSSL: assert result is None mock_secho.assert_any_call("SSL certificate verification failed: SSL verification failed", fg="red") - mock_secho.assert_any_call("Try setting REQUESTS_CA_BUNDLE environment variable to your CA bundle path", fg="yellow") + mock_secho.assert_any_call("Solutions:", fg="cyan") + mock_secho.assert_any_call(" 1. Set REQUESTS_CA_BUNDLE environment variable to your CA bundle path", fg="yellow") def test_fetch_provider_data_general_request_error(self): cache_file = Path("/tmp/test_cache.json")