fix: give PlatformAPIClient the contract APIDeploymentsClient already has

Six findings from the review on this PR. Five were real contract gaps and one
was a missed export; all are pinned by tests that fail when the fix is reverted.

**The body is read as JSON, not through the generated model.** This is the one
that mattered. `sync_detailed` reaches `_parse_response`, which does
`PlatformKeyError.from_dict(response.json())` on a 401 with no guard: a gateway
answering 401 with HTML raises `JSONDecodeError`, and a DRF-shaped
`{"detail": ...}` raises `KeyError: 'message'` -- both out of the generated
parser, before this facade sees the response. So a rejected key crashed instead
of being reported. The request is now issued from the generated `_get_kwargs`
and the body read through `APIDeploymentsClient._read_body`, which is exactly
why that helper exists.

This is the same defect class as the `_error_text` fix in the previous commit. I
fixed the half where reporting a refusal crashed and missed the half where
building the model crashed first.

**Transport failures are translated.** The class called `sync_detailed`
directly, so an unreachable host raised raw `httpx.ConnectError` -- contradicting
the module docstring this PR added, which promises the `requests` exception types
callers catch. It now goes through `_send`, like every deployment-key request.

**The credential is read per request.** `AuthenticatedClient` bakes its auth
header on first use, so a key assigned after the transport was built kept
sending the old one. `_send` sets the header per call.

**`close`, `__enter__` and `__exit__`** -- pooled connections had nothing to
release them, and the CLI builds one client per job.

**Re-exported from the package root**, so it is reachable as
`unstract.api_deployments.PlatformAPIClient` rather than only from the private
module.

Also corrected the `_error_text` comment from the previous commit: it described
the platform facade raising AttributeError, which is no longer a path that
exists now that both facades hand it an httpx response.

438 tests pass, up from 430. Five mutations killed: unguarded `response.json()`,
dropping the transport translation, capturing the key with the transport,
neutering `close`, and removing the re-export. The generated tree is untouched,
so `sdk-drift` is unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ

Author: praveen-formido

src/unstract/api_deployments/__init__.py

@@ -1,6 +1,7 @@
 __version__ = "1.6.0"
 
 from .client import APIDeploymentsClient as APIDeploymentsClient
+from .client import PlatformAPIClient as PlatformAPIClient
 
 
 def get_sdk_version():

src/unstract/api_deployments/client.py

@@ -176,10 +176,10 @@ def _error_text(body: Any, response) -> str:
             value = body.get(key)
             if isinstance(value, str) and value:
                 return value
-    # `.text` on an httpx response, `.content` on the generated `Response`
-    # wrapper -- which is an attrs class, not an httpx one, and carries only
-    # bytes. Without this the platform facade raises AttributeError while
-    # reporting a refusal, turning a 401 into a crash.
+    # Both facades hand this an httpx response, which has `.text`. The
+    # `.content` fallback is for the generated `Response` wrapper -- an attrs
+    # class carrying only bytes -- so passing one here reports the reason
+    # instead of raising AttributeError on the way to reporting it.
     text = getattr(response, "text", None)
     if text is None:
         text = (getattr(response, "content", b"") or b"").decode("utf-8", "replace")
@@ -953,7 +953,11 @@ class PlatformAPIClient:
     a platform key and address the account. Folding them together would mean a
     class whose required `api_url` is meaningless for half its methods.
 
-    Both credentials are HTTP bearer, so the generated transport is shared.
+    Everything else about the contract is deliberately the same as that class:
+    the request is issued through `_send`, so transport failures arrive as the
+    `requests` exception types callers already catch; the body is read as JSON
+    rather than through the generated response model; the credential is read per
+    request; and the pooled connections are released by `close`.
     """
 
     def __init__(
@@ -969,8 +973,8 @@ def __init__(
         Args:
             base_url (str): Scheme and host of the Unstract deployment, e.g.
                 ``https://us-central.unstract.com``. A path is ignored: these
-                operations carry their own, and the generated transport joins
-                them onto the origin.
+                operations carry their own, and the generated builders join them
+                onto the origin.
             api_key (str | None): Platform API key. Falls back to
                 ``$UNSTRACT_PLATFORM_KEY``, matching how `APIDeploymentsClient`
                 falls back for the deployment key.
@@ -1014,6 +1018,9 @@ def _transport(self) -> AuthenticatedClient:
         Same reasoning as `APIDeploymentsClient._transport`: two threads racing
         the first call would each build a pool and one would be dropped while
         still holding its sockets.
+
+        The token given here is not what authenticates a request -- `_send`
+        sets the header per call -- but `AuthenticatedClient` requires one.
         """
         if self._transport_client is None:
             with self._transport_lock:
@@ -1028,29 +1035,62 @@ def _transport(self) -> AuthenticatedClient:
                     )
         return self._transport_client
 
-    def _parsed_or_raise(self, response, what: str) -> Any:
-        """The parsed body of a 2xx, or an exception naming why it was refused.
+    def close(self) -> None:
+        """Release the pooled connections this client holds.
+
+        As on `APIDeploymentsClient`: the transport is kept between calls so
+        connections are reused, nothing else releases its sockets, and a client
+        built per job would otherwise accumulate pools. Safe to call twice.
+        """
+        with self._transport_lock:
+            transport, self._transport_client = self._transport_client, None
+        if transport is not None:
+            transport.get_httpx_client().close()
+
+    def __enter__(self) -> "PlatformAPIClient":
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback) -> None:
+        self.close()
+
+    def _send(self, method: str, url: str, **kwargs) -> httpx.Response:
+        """Issue one request, translating transport failures on the way out.
+
+        The credential is read per request rather than captured with the
+        transport, so assigning ``api_key`` takes effect on the next call --
+        `AuthenticatedClient` bakes its own header on first use, which would
+        otherwise pin whatever key the client was built with.
+        """
+        kwargs["headers"] = {
+            **(kwargs.get("headers") or {}),
+            "Authorization": f"Bearer {self.api_key}",
+        }
+        return _translate_transport_errors(
+            self._transport.get_httpx_client().request, method, url, **kwargs
+        )
+
+    def _read_or_raise(self, response: httpx.Response, what: str) -> Any:
+        """The JSON body of a 2xx, or an exception naming why it was refused.
 
-        `raise_on_unexpected_status` is off on the shared transport, so a
-        refusal arrives as an ordinary response. Without this every caller would
-        have to re-derive that check, and a 401 would read as an empty result.
+        The body is read directly rather than through the generated response
+        model, for the reason `APIDeploymentsClient._read_body` gives: a model
+        is built only for the statuses the spec declares, and its `from_dict`
+        indexes required keys with no default. A gateway answering 401 with HTML,
+        or the API answering with a DRF-shaped ``{"detail": ...}``, would raise
+        `JSONDecodeError` or `KeyError` out of the generated parser -- before
+        this facade ever sees the response -- instead of reporting a refusal.
         """
+        body = APIDeploymentsClient._read_body(response)
         if not 200 <= response.status_code < 300:
-            # `parsed` is a generated model, and `_error_text` reads mappings;
-            # handed the model it would fall through to the raw body and drop
-            # the reason the endpoint actually sent.
-            body = response.parsed
-            if hasattr(body, "to_dict"):
-                body = body.to_dict()
             raise APIDeploymentsClientException(
                 f"{what} failed with {response.status_code}: "
                 f"{_error_text(body, response)}"
             )
-        if response.parsed is None:
+        if body is None:
             raise APIDeploymentsClientException(
-                f"{what} returned {response.status_code} with no readable body."
+                f"{what} returned {response.status_code} with a body that is not JSON."
             )
-        return response.parsed
+        return body
 
     def whoami(self) -> dict:
         """The organisation this key belongs to, and the key's own scope.
@@ -1064,8 +1104,9 @@ def whoami(self) -> dict:
                 and ``key_name``.
         """
         self.logger.debug("Resolving identity via /unstract/whoami/")
-        response = whoami.sync_detailed(client=self._transport)
-        return self._parsed_or_raise(response, "whoami").to_dict()
+        request_kwargs = whoami._get_kwargs()
+        response = self._send(**request_kwargs)
+        return self._read_or_raise(response, "whoami")
 
     def list_deployments(
         self,
@@ -1099,14 +1140,14 @@ def list_deployments(
             dict: ``count``, ``next``, ``previous`` and ``results``.
         """
         self.logger.debug("Listing deployments for organisation: " + org_id)
-        response = list_deployments.sync_detailed(
+        request_kwargs = list_deployments._get_kwargs(
             org_id,
-            client=self._transport,
             api_name=api_name,
             search=search,
             ordering=ordering,
             page=page,
             page_size=page_size,
             workflow=workflow,
         )
-        return self._parsed_or_raise(response, "list_deployments").to_dict()
+        response = self._send(**request_kwargs)
+        return self._read_or_raise(response, "list_deployments")

tests/test_compat.py

@@ -2025,9 +2025,14 @@ def test_whoami_returns_the_four_fields_the_spec_declares():
         result = _platform_client().whoami()
 
     assert result == identity
-    url = transport.request.call_args.kwargs["url"]
+    # Issued through `_send`, which passes method and url positionally the way
+    # the sibling client does.
+    method, url = transport.request.call_args.args[:2]
+    assert method == "get", method
     # No organisation segment: putting one there would defeat the point.
     assert url.endswith("/api/v1/unstract/whoami/"), url
+    sent = transport.request.call_args.kwargs["headers"]["Authorization"]
+    assert sent == "Bearer pk-test", sent
 
 
 def test_list_deployments_sends_the_organisation_and_reads_the_page():
@@ -2060,9 +2065,9 @@ def test_list_deployments_sends_the_organisation_and_reads_the_page():
 
     assert result["count"] == 1
     assert result["results"][0]["api_name"] == "invoice-parser"
-    called = transport.request.call_args.kwargs
-    assert "/org-a/" in called["url"], called["url"]
-    assert called["params"]["api_name"] == "invoice-parser"
+    url = transport.request.call_args.args[1]
+    assert "/org-a/" in url, url
+    assert transport.request.call_args.kwargs["params"]["api_name"] == "invoice-parser"
 
 
 def test_a_missing_platform_key_is_refused_at_construction():
@@ -2091,3 +2096,100 @@ def test_a_path_on_the_base_url_is_discarded():
         base_url="https://example.unstract.com/deployment/api/x/y/"
     )
     assert client.base_url == "https://example.unstract.com"
+
+
+# The findings below were raised on PR #29 and each fix is pinned here, so a
+# regression to the pre-review behaviour fails rather than passing quietly.
+
+
+@pytest.mark.parametrize(
+    ("label", "body_text"),
+    [
+        ("gateway_html", "<html><body>401 Unauthorized</body></html>"),
+        ("drf_shaped", '{"detail": "Invalid token."}'),
+        ("empty", ""),
+    ],
+)
+def test_an_error_body_the_model_cannot_parse_is_still_reported(label, body_text):
+    """The generated `_parse_response` builds `PlatformKeyError.from_dict(...)`
+    on a 401 with no guard: HTML raises `JSONDecodeError` and a DRF-shaped body
+    raises `KeyError: 'message'`, both out of the parser and before the facade
+    sees the response. Reading the body directly is what keeps a refused key a
+    reported refusal rather than a crash.
+    """
+    transport = MagicMock()
+    transport.request.return_value = httpx.Response(401, text=body_text)
+    with patch.object(AuthenticatedClient, "get_httpx_client", return_value=transport):
+        with pytest.raises(APIDeploymentsClientException) as caught:
+            _platform_client().whoami()
+    assert "401" in str(caught.value), label
+
+
+def test_a_transport_failure_arrives_as_the_requests_exception(monkeypatch):
+    """`APIDeploymentsClient` routes every request through
+    `_translate_transport_errors` so callers catch the `requests` classes they
+    document. Calling the generated `sync_detailed` directly would let raw
+    `httpx.ConnectError` escape, contradicting the module docstring.
+    """
+    transport = MagicMock()
+    transport.request.side_effect = httpx.ConnectError("nope")
+    with patch.object(AuthenticatedClient, "get_httpx_client", return_value=transport):
+        with pytest.raises(ConnectionError):
+            _platform_client().whoami()
+
+
+def test_the_platform_key_is_read_per_request_not_captured():
+    """`AuthenticatedClient` bakes its auth header on first use, so a key
+    reassigned after the transport was built would silently keep sending the
+    old one. `_send` sets the header per call for exactly this reason.
+    """
+    identity = {
+        "organization_id": "org-a",
+        "organization_name": "Org A",
+        "permission": "read",
+        "key_name": "k",
+    }
+    client = _platform_client(api_key="pk-first")
+    transport = MagicMock()
+    transport.request.return_value = _httpx_response(200, identity)
+    with patch.object(AuthenticatedClient, "get_httpx_client", return_value=transport):
+        client.whoami()
+        first = transport.request.call_args.kwargs["headers"]["Authorization"]
+        client.api_key = "pk-second"
+        client.whoami()
+        second = transport.request.call_args.kwargs["headers"]["Authorization"]
+
+    assert first == "Bearer pk-first"
+    assert second == "Bearer pk-second"
+
+
+def test_close_releases_the_pool_and_the_next_call_rebuilds_it():
+    """Nothing else releases the transport's sockets, and the CLI builds one
+    client per job."""
+    client = _platform_client()
+    inner = MagicMock()
+    with patch.object(AuthenticatedClient, "get_httpx_client", return_value=inner):
+        assert client._transport is not None
+        client.close()
+        inner.close.assert_called_once()
+    assert client._transport_client is None
+    # Safe twice, and idempotent.
+    client.close()
+
+
+def test_the_platform_client_is_a_context_manager():
+    with patch.object(
+        AuthenticatedClient, "get_httpx_client", return_value=MagicMock()
+    ):
+        with _platform_client() as client:
+            assert client._transport is not None
+        assert client._transport_client is None
+
+
+def test_both_clients_are_reachable_from_the_package_root():
+    """A class only importable from the private module is not a published
+    surface, and the sibling is re-exported."""
+    from unstract import api_deployments as pkg
+
+    assert pkg.PlatformAPIClient is PlatformAPIClient
+    assert pkg.APIDeploymentsClient is APIDeploymentsClient