🟡 Changes recommended

HTTP CONNECT status parsing must be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Suppressed comments (1)

test/asynchronous/test_encryption.py:19

• The KMS callback tests were moved to test_kms_connect.py, but their newly added imports remain here unused (asyncio, threading, time, TransportSocket, mock, the KMS helpers/private encryption symbols, PoolOptions, and get_ssl_context). Remove them from this asynchronous source test and regenerate the synchronous mirror to avoid stale dependencies and import-time overhead.

import asyncio

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Balanced

🟡 Changes recommended

Critical callback-validation and request-injection issues must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pymongo/asynchronous/encryption.py:192

• This restores the original opts.socket_timeout after the callback, so time spent establishing the proxy tunnel is not deducted before the KMS TLS handshake. Under automatic encryption with timeoutMS, a callback can consume nearly the entire remaining budget and the handshake then receives that budget again, allowing the operation to overrun its deadline. Track a deadline across callback execution and TLS wrapping, or recompute the remaining CSOT budget before the handshake.

pymongo/asynchronous/encryption.py:175

• The async error directs users to HTTPProxyKMSConnect, but that synchronous helper is incompatible with this API and can block the event loop before being rejected. Name AsyncHTTPProxyKMSConnect in the async source; the synchro mapping will produce the synchronous helper name in the generated module.

        raise ConfigurationError(
            "kms_connect_callback must return a connected, unwrapped "
            f"socket.socket, not {type(sock)}; consider HTTPProxyKMSConnect."

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Balanced

🟡 Changes recommended

Five moderate timeout/deadline and HTTP status-validation issues must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pymongo/encryption_options.py:176

• HTTP status codes are exactly three digits, but this accepts malformed codes such as 00200 as success and can call int() on thousands of digits, raising ValueError instead of the intended proxy OSError. Require a three-digit status code before converting it.

pymongo/encryption_options.py:235

• socket.create_connection applies this timeout separately to every address returned by DNS. For a dual-stack or multi-address proxy, each unreachable address can consume the entire remaining budget, so this call can exceed the KMS/CSOT deadline by a multiple. Resolve and try addresses with _remaining(deadline) recomputed for each attempt instead of delegating the whole loop to create_connection.

        sock = socket.create_connection((self.host, self.port), timeout=_remaining(deadline))

• Files reviewed: 12/12 changed files
• Comments generated: 3
• Review effort level: Balanced

🟡 Changes recommended

Async TLS-handshake cancellation can leak sockets and proxy threads; async tests also contain blocking socket operations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Suppressed comments (3)

test/asynchronous/test_kms_connect.py:315

• sendall and recv are blocking socket calls executed directly by this coroutine; if the delayed proxy response never arrives, the event loop is blocked for up to 10 seconds. Offload them with run_in_executor (while retaining the sync branch for synchro generation).

        sock.settimeout(10)
        sock.sendall(b"ping")
        self.assertEqual(sock.recv(64), b"echo:ping")

test/asynchronous/test_kms_connect.py:269

• These synchronous socket operations run inside an async test and can block the event-loop thread for up to the configured 10-second timeout when the relay stalls. Run both operations in an executor (as proxy_request does below) so failures do not freeze the async test loop.

        sock.settimeout(10)
        sock.sendall(b"ping")
        self.assertEqual(sock.recv(64), b"echo:ping")

test/asynchronous/test_kms_connect.py:379

• This blocking recv is invoked on the event-loop thread and can stall the entire async test for up to 10 seconds if the expected coalesced bytes are delayed. Await an executor-backed receive instead, following the pattern used by proxy_request.

        sock.settimeout(10)
        self.assertEqual(sock.recv(64), b"early-bytes")

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Balanced

🔵 Needs a closer look

The HTTPS prose tests disable required certificate verification, and some new API documentation is incomplete.

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

test/asynchronous/test_kms_connect.py:626

• This disables both certificate and hostname verification for the HTTPS proxy, so prose Case 2 can pass with an untrusted or wrong-host proxy certificate. The specification requires the callback's proxy TLS connection to be verified with ca.pem; keep the verification defaults established by create_default_context.
  test/asynchronous/test_kms_connect.py:642
• The HTTPS control requests also disable the ca.pem certificate and hostname checks, contrary to the prose setup for Case 2. Leaving the default context verification enabled ensures reset/metrics calls fail when the proxy presents an invalid identity.
  test/asynchronous/test_kms_connect.py:728
• This only proves that some KMS connection occurred. Prose Case 3 also expects exactly one request after the reset, verifying that the subsequent decrypt reuses the cached key; asserting equality would cover that required behavior.
  pymongo/asynchronous/encryption.py:696
• This async API rejects ordinary callables before invoking them, but the new parameter documentation says any callable is accepted. Document the coroutine requirement so users do not pass the synchronous helper or a blocking function; phrasing it with async def also lets synchro generate the corresponding sync wording.
  pymongo/encryption_options.py:124
• The example imports only HTTPProxyKMSConnect but immediately constructs AutoEncryptionOpts, so it cannot run as shown. Import both public classes in the snippet.

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Balanced