Bump to websockets==15.0.1#92
Open
Conversation
Code Review by Qodo (Alpha)
1. External echo server in test
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+161
to
+172
| async def test_websocket_custom_headers_with_websocket_connect(): | ||
| """Test that custom headers work with the websocket_connect utility function""" | ||
|
|
||
| custom_headers = {"X-Custom-Header": "test-value"} | ||
|
|
||
| with temporary_settings({PREFECT_CLIENT_CUSTOM_HEADERS: custom_headers}): | ||
| connector = websocket_connect(events_in_socket_from_api_url(hosted_api_server)) | ||
| # Make sure we can connect to the websocket successfully with the custom headers | ||
| # Connect to actual WebSocket echo server to verify custom headers work | ||
| connector = websocket_connect("wss://echo.websocket.org") | ||
| async with connector as websocket: | ||
| pong = await websocket.ping() | ||
| await pong | ||
|
|
||
| # Check that custom headers are in additional_headers | ||
| assert "additional_headers" in connector._kwargs | ||
| additional_headers = connector._kwargs["additional_headers"] | ||
| assert additional_headers["X-Custom-Header"] == "test-value" | ||
| await websocket.send("test message") | ||
| response = await websocket.recv() | ||
| assert response == "test message" |
There was a problem hiding this comment.
1. External echo server in test 📘 Rule Violation
• The new test test_websocket_custom_headers_with_websocket_connect connects to wss://echo.websocket.org, making the test suite depend on an external network service. • This introduces nondeterminism (network/DNS/outage/flaky behavior) and can fail in CI environments without outbound network access, violating the requirement to mock external dependencies.
Agent Prompt
## Issue description
A test connects to a real external WebSocket server (`wss://echo.websocket.org`), making it nondeterministic and dependent on network availability.
## Issue Context
Compliance requires tests to be deterministic and to mock/stub external dependencies.
## Fix Focus Areas
- tests/_internal/test_websockets.py[161-172]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+86
| # Add custom headers from settings | ||
| custom_headers = get_current_settings().client.custom_headers | ||
| if custom_headers: | ||
| # Get existing additional_headers or create new dict | ||
| additional_headers = {} | ||
| if not isinstance(additional_headers, dict): | ||
| additional_headers = {} | ||
|
|
||
| for header_name, header_value in custom_headers.items(): | ||
| # Check for protected headers that shouldn't be overridden | ||
| if header_name in { | ||
| "user-agent", | ||
| "sec-websocket-key", | ||
| "sec-websocket-version", | ||
| "sec-websocket-extensions", | ||
| "sec-websocket-protocol", | ||
| "connection", | ||
| "upgrade", | ||
| "host", | ||
| }: | ||
| warnings.warn( | ||
| f"Custom header '{header_name}' is ignored because it conflicts with " | ||
| f"a protected WebSocket header. Protected headers include: " | ||
| f"User-Agent, Sec-WebSocket-Key, Sec-WebSocket-Version, " | ||
| f"Sec-WebSocket-Extensions, Sec-WebSocket-Protocol, Connection, " | ||
| f"Upgrade, Host", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
| else: | ||
| additional_headers[header_name] = header_value | ||
|
|
||
| kwargs["additional_headers"] = additional_headers |
There was a problem hiding this comment.
2. websocket_connect header merge bug 📘 Rule Violation
• websocket_connect claims to “Get existing additional_headers”, but it always initializes
additional_headers = {} and never reads kwargs.get("additional_headers"), so caller-provided
headers can be dropped rather than merged.
• The protected-header check is case-sensitive (if header_name in {"user-agent", ...}) and does
not normalize case, allowing protected headers like User-Agent to bypass the protection logic and
be forwarded/overridden.
• This is missing edge-case handling for common header casing and weakens protection against
unsafe/invalid header overrides.
Agent Prompt
## Issue description
`websocket_connect` does not actually merge existing `additional_headers` from `kwargs`, and the protected-header check is case-sensitive, allowing common-cased protected headers (e.g., `User-Agent`) to bypass restrictions.
## Issue Context
This logic processes potentially user-configured headers and must robustly handle common casing and existing caller-provided headers.
## Fix Focus Areas
- src/prefect/_internal/websockets.py[54-86]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
19
to
23
| def test_websocket_connect_factory(): | ||
| """Test that websocket_connect returns WebsocketProxyConnect instance""" | ||
| """Test that websocket_connect returns a connect instance""" | ||
| connector = websocket_connect("wss://example.com") | ||
| assert isinstance(connector, WebsocketProxyConnect) | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_invalid_uri_raises_value_error(): | ||
| """Test that invalid URI schemes raise appropriate errors""" | ||
| with pytest.raises(ValueError, match="Invalid URI"): | ||
| WebsocketProxyConnect("invalid-uri") | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_unsupported_scheme_raises_value_error(): | ||
| """Test that unsupported URI schemes raise appropriate errors""" | ||
| with pytest.raises(ValueError, match="Unsupported scheme"): | ||
| WebsocketProxyConnect("ftp://example.com") | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_ws_scheme(): | ||
| """Test WebSocket proxy connect with ws:// scheme""" | ||
| connector = WebsocketProxyConnect("ws://example.com") | ||
| assert connector._host == "example.com" | ||
| assert connector._port == 80 | ||
| assert connector.uri == "ws://example.com" | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_wss_scheme(): | ||
| """Test WebSocket proxy connect with wss:// scheme""" | ||
| connector = WebsocketProxyConnect("wss://example.com") | ||
| assert connector._host == "example.com" | ||
| assert connector._port == 443 | ||
| assert connector.uri == "wss://example.com" | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_custom_port(): | ||
| """Test WebSocket proxy connect with custom port""" | ||
| connector = WebsocketProxyConnect("wss://example.com:8443") | ||
| assert connector._host == "example.com" | ||
| assert connector._port == 8443 | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_with_http_proxy(): | ||
| """Test proxy configuration with HTTP_PROXY environment variable""" | ||
| old_proxy = os.environ.get("HTTP_PROXY") | ||
| os.environ["HTTP_PROXY"] = "http://proxy.example.com:8080" | ||
|
|
||
| try: | ||
| connector = WebsocketProxyConnect("ws://example.com") | ||
| assert connector._proxy_url is not None | ||
| finally: | ||
| if old_proxy: | ||
| os.environ["HTTP_PROXY"] = old_proxy | ||
| elif "HTTP_PROXY" in os.environ: | ||
| del os.environ["HTTP_PROXY"] | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_with_https_proxy(): | ||
| """Test proxy configuration with HTTPS_PROXY environment variable""" | ||
| old_proxy = os.environ.get("HTTPS_PROXY") | ||
| os.environ["HTTPS_PROXY"] = "http://proxy.example.com:8080" | ||
|
|
||
| try: | ||
| connector = WebsocketProxyConnect("wss://example.com") | ||
| assert connector._proxy_url is not None | ||
| finally: | ||
| if old_proxy: | ||
| os.environ["HTTPS_PROXY"] = old_proxy | ||
| elif "HTTPS_PROXY" in os.environ: | ||
| del os.environ["HTTPS_PROXY"] | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_proxy_bypass(): | ||
| """Test that proxy bypass logic works correctly""" | ||
| old_proxy = os.environ.get("HTTPS_PROXY") | ||
| os.environ["HTTPS_PROXY"] = "http://proxy.example.com:8080" | ||
|
|
||
| try: | ||
| with patch("prefect._internal.websockets.proxy_bypass", return_value=True): | ||
| connector = WebsocketProxyConnect("wss://example.com") | ||
| assert connector._proxy_url is None | ||
| finally: | ||
| if old_proxy: | ||
| os.environ["HTTPS_PROXY"] = old_proxy | ||
| elif "HTTPS_PROXY" in os.environ: | ||
| del os.environ["HTTPS_PROXY"] | ||
| assert isinstance(connector, connect) | ||
|
|
There was a problem hiding this comment.
3. Invalid isinstance assertion 🐞 Bug
• Tests use assert isinstance(connector, connect) where connect is imported as a callable symbol, not a type. • In Python, isinstance(..., <non-type>) raises TypeError, so this assertion can error instead of validating behavior. • Impact: test failures unrelated to product behavior; brittle across websockets versions/exports.
Agent Prompt
### Issue description
`isinstance(connector, connect)` is not a stable/correct type assertion because `connect` is imported as a callable symbol and may not be a type.
### Issue Context
These tests should validate Prefect’s wrapper behavior (kwargs handling) without depending on websockets’ exported symbol types.
### Fix Focus Areas
- tests/_internal/test_websockets.py[19-23]
- tests/_internal/test_websockets.py[49-78]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
78
| def test_websocket_connect_ssl_integration(): | ||
| """Test that websocket_connect integrates with SSL context creation""" | ||
| with temporary_settings({PREFECT_API_TLS_INSECURE_SKIP_VERIFY: True}): | ||
| connector = WebsocketProxyConnect("wss://example.com") | ||
| # SSL context should be in kwargs | ||
| assert "ssl" in connector._kwargs | ||
| ssl_context = connector._kwargs["ssl"] | ||
| connector = websocket_connect("wss://example.com") | ||
| assert isinstance(connector, connect) | ||
| # Verify SSL context is configured | ||
| assert "ssl" in connector.connection_kwargs | ||
| ssl_context = connector.connection_kwargs["ssl"] | ||
| assert not ssl_context.check_hostname | ||
| assert ssl_context.verify_mode == ssl.CERT_NONE | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_no_ssl_for_ws(): | ||
| """Test that WebsocketProxyConnect doesn't add SSL for ws:// URLs""" | ||
| connector = WebsocketProxyConnect("ws://example.com") | ||
| assert "ssl" not in connector._kwargs | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_server_hostname(): | ||
| """Test that server_hostname is set for wss:// connections""" | ||
| connector = WebsocketProxyConnect("wss://example.com") | ||
| assert "server_hostname" in connector._kwargs | ||
| assert connector._kwargs["server_hostname"] == "example.com" | ||
|
|
||
| def test_websocket_connect_no_ssl_for_ws(): | ||
| """Test that websocket_connect doesn't add SSL for ws:// URLs""" | ||
| connector = websocket_connect("ws://example.com") | ||
| assert isinstance(connector, connect) | ||
| # Verify SSL is not configured for ws:// URLs | ||
| assert "ssl" not in connector.connection_kwargs | ||
|
|
||
| def test_websocket_proxy_connect_no_server_hostname_for_ws(): | ||
| """Test that server_hostname is not set for ws:// connections""" | ||
| connector = WebsocketProxyConnect("ws://example.com") | ||
| assert "server_hostname" not in connector._kwargs | ||
|
|
||
|
|
||
| def test_websocket_proxy_connect_kwargs_preservation(): | ||
| def test_websocket_connect_kwargs_preservation(): | ||
| """Test that additional kwargs are preserved""" | ||
| additional_headers = {"Authorization": "Bearer token"} | ||
| connector = WebsocketProxyConnect( | ||
| connector = websocket_connect( | ||
| "wss://example.com", additional_headers=additional_headers | ||
| ) | ||
| assert connector._kwargs["additional_headers"] == additional_headers | ||
| assert isinstance(connector, connect) | ||
| # Verify headers are preserved | ||
| assert connector.additional_headers == additional_headers | ||
|
|
There was a problem hiding this comment.
4. Tests use connect internals 🐞 Bug
• Tests (and one events-client test) read connector.connection_kwargs / connector.additional_headers, which are third-party implementation details. • This can break on minor websockets upgrades even if Prefect behavior is correct. • Impact: fragile tests and higher maintenance for dependency bumps.
Agent Prompt
### Issue description
Tests depend on non-Prefect attributes (`connection_kwargs`, `additional_headers`) on the websockets connect object.
### Issue Context
Dependency upgrades can change these attributes even if runtime behavior remains correct.
### Fix Focus Areas
- tests/_internal/test_websockets.py[49-78]
- tests/events/client/test_events_client.py[69-87]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from qodo-benchmark#273