[#36] feat: add HTTP client transport support#37
Conversation
Add HttpClientTransport for communicating with ADP servers over HTTP, enabling connections to remote or already-running servers without managing a subprocess. Changes: - Add httpx>=0.27.1 as a required dependency - Implement HttpClientTransport (POST-based JSON-RPC request/response) - Add http_client() async context manager convenience function - Add 17 unit tests covering lifecycle, messages, and error responses - Update README.md with HTTP transport documentation and examples Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an HTTP-based client transport to the ADP Python SDK so clients can talk to remote/already-running ADP servers without spawning a subprocess.
Changes:
- Introduces
HttpClientTransport(POST-based JSON-RPC) and re-exports it from the transports package. - Adds
http_client()async context manager for creating an initializedClientSessionover HTTP. - Updates dependencies/docs and adds a dedicated unit test suite for the new transport.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Moves httpx into required deps and bumps lock revision/version metadata. |
| pyproject.toml | Adds httpx>=0.27.1 as a required dependency. |
| src/adp_sdk/transports/http/client.py | Implements HttpClientTransport with request/response buffering semantics. |
| src/adp_sdk/transports/http/init.py | Exposes HttpClientTransport from the http transport package. |
| src/adp_sdk/transports/init.py | Re-exports HttpClientTransport at adp_sdk.transports. |
| src/adp_sdk/init.py | Adds http_client() convenience context manager and exports HttpClientTransport. |
| tests/test_http_client.py | Adds unit tests for HTTP transport lifecycle, headers, message flow, and error bodies. |
| README.md | Documents HTTP transport usage alongside stdio transport. |
| .gitignore | Ignores JetBrains .idea/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| authorization: Optional authorization header value for requests | ||
| (e.g. ``basic_auth("user", "pass")``). When provided, it is | ||
| sent as the HTTP ``Authorization`` header so the server can | ||
| extract it at the transport level. | ||
|
|
||
| Yields: | ||
| An initialized :class:`ClientSession`. | ||
| """ | ||
| effective_headers = dict(headers) if headers else {} | ||
| if authorization is not None: |
There was a problem hiding this comment.
http_client() silently overwrites any user-provided Authorization header when both headers (with an Authorization key) and the authorization argument are provided. Consider either (a) raising a ValueError when both are set, or (b) documenting/implementing a clear precedence rule (e.g., only set the header if it’s not already present).
| authorization: Optional authorization header value for requests | |
| (e.g. ``basic_auth("user", "pass")``). When provided, it is | |
| sent as the HTTP ``Authorization`` header so the server can | |
| extract it at the transport level. | |
| Yields: | |
| An initialized :class:`ClientSession`. | |
| """ | |
| effective_headers = dict(headers) if headers else {} | |
| if authorization is not None: | |
| Must not include ``Authorization`` when ``authorization`` is also | |
| provided. | |
| authorization: Optional authorization header value for requests | |
| (e.g. ``basic_auth("user", "pass")``). When provided, it is sent | |
| as the HTTP ``Authorization`` header so the server can extract it | |
| at the transport level. Must not be combined with an | |
| ``Authorization`` entry in ``headers``. | |
| Yields: | |
| An initialized :class:`ClientSession`. | |
| Raises: | |
| ValueError: If both ``authorization`` and ``headers["Authorization"]`` | |
| are provided. | |
| """ | |
| effective_headers = dict(headers) if headers else {} | |
| if authorization is not None: | |
| if "Authorization" in effective_headers: | |
| raise ValueError( | |
| "Provide either headers['Authorization'] or authorization, not both." | |
| ) |
There was a problem hiding this comment.
Fixed in 22b39de. http_client() now raises ValueError when authorization= is combined with an existing Authorization header in headers, and the check is case-insensitive. I also updated the docstring and added a regression test for this path.
| headers = {**self._headers, "Content-Type": "application/json"} | ||
| response = await self._client.post(self._url, content=message, headers=headers) | ||
|
|
||
| self._pending_response = response.text | ||
| logger.debug("HTTP %s %s -> %d", "POST", self._url, response.status_code) |
There was a problem hiding this comment.
write_message() will overwrite _pending_response if it’s called twice without an intervening read_message(), silently dropping the previous HTTP response. Since this transport is explicitly request/response paired, consider raising a RuntimeError (or similar) when _pending_response is still set to enforce correct sequencing and avoid hard-to-debug response/id mismatches when HttpClientTransport is used directly (outside ClientSession).
There was a problem hiding this comment.
Fixed in 22b39de. HttpClientTransport.write_message() now raises RuntimeError if the previous response has not been consumed yet, so we no longer overwrite _pending_response. Added a regression test for direct transport usage as well.
There was a problem hiding this comment.
Where did you fix the things?
There was a problem hiding this comment.
This should not happen through ClientSession, since ClientSession._send_request() serializes calls with _send_lock and always performs write_message() followed immediately by read_message().
The new RuntimeError only triggers when HttpClientTransport is used directly with an invalid write -> write sequence while the previous response is still buffered in _pending_response.
src/adp_sdk/transports/http/client.py:103-106adds that guard.tests/test_http_client.py:241-247adds the regression test for the direct transport usage path.
There was a problem hiding this comment.
I mean where is the commit id 22b39de, I don't see this in the PR.
There was a problem hiding this comment.
You were right — 22b39de had been pushed to the wrong remote earlier. I have now pushed it to the PR branch (mchades/python-sdk:http-transport), so that commit is part of this PR now.
|
|
||
|
|
||
| @asynccontextmanager | ||
| async def http_client( |
There was a problem hiding this comment.
Do we have the concurrency requirement? This client seems can only work in a single thread.
There was a problem hiding this comment.
Current HttpClientTransport is intentionally a single in-flight request/response transport, which matches the existing SDK transport contract.
Transport already documents that read_message() / write_message() are not concurrent-safe, and ClientSession serializes concurrent requests with an internal asyncio.Lock() to prevent response mismatches.
So this PR keeps the current client model consistent rather than introducing a new concurrency limitation specific to HTTP. If we want true multiple in-flight HTTP requests, that would likely require a broader transport/session design change rather than a small adjustment in this PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What changes were proposed in this pull request?
Add
HttpClientTransportfor communicating with ADP servers over HTTP, enabling connections to remote or already-running servers without managing a subprocess.Changes:
httpx>=0.27.1as a required dependency (following MCP SDK precedent)HttpClientTransportinsrc/adp_sdk/transports/http/(POST-based JSON-RPC request/response)http_client()async context manager convenience function insrc/adp_sdk/__init__.pysrc/adp_sdk/transports/__init__.pyto re-exportHttpClientTransportREADME.mdwith HTTP transport documentation and examplesWhy are the changes needed?
The adp-hypervisor is adding HTTP transport support (adp-hypervisor#89). The SDK needs a corresponding client-side HTTP transport so that clients can connect to ADP servers running in HTTP mode.
Fix: #36
Does this PR introduce any user-facing change?
Yes:
HttpClientTransportfor HTTP-based communication.http_client()— async context manager that creates an initializedClientSessionover HTTP.httpx>=0.27.1is now a required dependency.How was this patch tested?
tests/test_http_client.pycovering:jsonschemaissue)ruff check✅,ruff format✅,pyright0 errors ✅