feat: pick up the whoami and deployment-listing operations - #29
Merged
chandrasekharan-zipstack merged 4 commits intoSep 10, 2026
Conversation
Runs the `spec-upgrade` pipeline against Zipstack/unstract main at 520b98d7a, which merged the two operations the CLI needs (unstract#2269 and unstract#2278). The spec is copied byte-for-byte and `SPEC_SOURCE` moves with it in this same commit — revision and sha256 both — so a current copy stays distinguishable from one the backend has moved past. I verified the *previous* record before moving it: the vendored file matched its recorded sha256 and was byte-identical to the backend at `eddd4b746`, so this upgrade starts from an honest baseline. Regeneration is purely additive: new `api/identity/whoami.py` and `api/deployment/list_deployments.py`, six new models, and prose-only changes to `execute`/`status` where #2278 reworded the descriptions. No operation, field or model was removed, so this is a **minor** bump rather than a major one. The generator exited clean, and regenerating a second time produces byte-identical output, so `sdk-drift` will pass. **The new facade class.** `whoami` and `list_deployments` both authenticate with a platform key, and neither fits `APIDeploymentsClient`: that class takes a *deployment* URL and derives an organisation and API name from its last two segments, which `whoami` has neither of. So `PlatformAPIClient` sits alongside it, sharing the generated transport — both schemes are HTTP bearer, only the token differs — and raising the same exception type. Folding them together would have meant a class whose required `api_url` is meaningless for half its methods. Without this the operations are generated but unreachable, and `unstract-cli` keeps reaching into `unstract.clone.PlatformClient` — the org-cloning tool's hand-written admin client — which is how the CLI drifted off the generated surface to begin with. **A defect the new tests caught.** `_error_text` fell back to `response.text`, but the generated `Response` is an attrs wrapper carrying `.content`, and `parsed` is a model instance rather than a mapping. A 401 through the new client would have raised `AttributeError` while trying to report the refusal. Both halves are fixed; the `_error_text` change is additive, so httpx callers are unaffected. **Test coverage.** `test_every_declared_operation_is_wrapped` fired exactly as designed when the spec grew. Extending `WRAPPED_OPERATIONS` was not the right answer, though: `whoami` declares no `ErrorResponse`, so that suite's "both families are in play" assertion is false for it by construction, and `_declared_responses` indexes `content` unconditionally, which its bodyless 500 would `KeyError` on. A parallel `PLATFORM_OPERATIONS` manifest carries its own status pins and error-reporting coverage, and the whole-set comparison now unions the two — so an operation belonging to neither still fails there, which is the property that test exists for. `__version__` and the compat baseline are deliberately untouched: the release workflow reads the former as the last released version and applies the bump at dispatch, and a spec upgrade is not a reason to move the parity reference point. 430 tests pass, up from 419. `ruff check` and `format --check` clean on everything this touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
… 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
praveen-formido
marked this pull request as ready for review
September 10, 2026 11:22
Contributor
|
Greptile, on PR #29. Two real problems, and the first is mine twice over. **A stale docstring.** `_platform_reply` claimed that patching at `get_httpx_client` meant "the generated parsing and model construction still run". That was true of the first commit, which called `sync_detailed`. The review fixes moved the facade to `_get_kwargs()` plus its own body read, and the sentence survived the change it described -- the same failure mode as the `openapi_schema` docstring this PR's backend counterpart had to fix. **And the coverage the sentence was standing in for did not exist.** With the facade reading bodies itself, nothing exercised `whoami._parse_response`, `list_deployments._parse_response`, or any of the six new models. A regeneration that broke them would have passed this suite. Added, exercised directly rather than through the facade: - every field of `WhoAmIResponse`, `PlatformKeyError` and the paginated listing, including the nested `APIDeploymentSummary` row; - both new `_parse_response` functions, on a declared 200 and a declared 401; - and the reason the facade does not use them -- a gateway's HTML 401 raises `ValueError` and a DRF-shaped body raises `KeyError` out of the parser. Pinning that keeps the facade's decision justified instead of looking arbitrary. The tier field is a correction too: the spec declares a ChoiceField, and I had described that as giving the client "a real enum". This generator renders it as a `Literal` alias plus a `check_api_key_permission` validator, so the value stays a plain string. The test now asserts what is actually emitted, and exercises the validator in both directions. `_deployment_page()` is shared between the facade test and the model tests: a row that satisfied one and not the other would prove nothing about either. 441 tests pass, up from 438. Two mutations killed to confirm the new coverage is real -- a model reading the wrong key, and a parser dropping its 401 branch. An earlier mutation of mine (adding a default to a required `d.pop`) survived because the test supplies the field, so it was equivalent rather than a miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ
chandrasekharan-zipstack
added a commit
that referenced
this pull request
Sep 10, 2026
PR #29 gave the platform client its own copy of the transport: the pool, the close/reopen handling and the httpx-to-requests exception translation were duplicated from APIDeploymentsClient, and the copy had no retry policy at all. Two copies of that code drift; only one of them was getting fixes. Both clients now inherit `_HttpxFacade`, which owns the lazily built pooled transport, `close`/context-manager support, the exception translation and the retry-with-Retry-After policy. A client subclass supplies its error class and its methods, nothing else. That drops ~184 duplicated lines and gives the platform operations the retry behaviour the README already promised. Three defects fall out of sharing the code: - The transport pool is now built inside the lock. `AuthenticatedClient .get_httpx_client()` builds lazily and unsynchronised, so publishing the client before warming it let two threads build two pools. - A close during flight no longer escapes untranslated. httpx answers a send on a closed client with a bare `RuntimeError`, which is not in the subtree `_translate_transport_errors` covers, so it reached callers catching the documented `requests` types. It is translated at the send. - `list_deployments` no longer sends `workflow=None`. The generated builder renders that parameter with `str()` before it filters `None` out, so the literal string "None" went on the wire as a filter matching no workflow on every otherwise unfiltered call. Unset filters are omitted instead, which also holds if the generator special-cases another parameter later. Exceptions get a hierarchy. `APIDeploymentsClientException` never worked -- its `__init__` nested three more `def`s that were never bound to the class, so `message` was dropped and `Exception.__init__` was never called, leaving `str(e)` empty and the documented `error_message()` non-existent. It is now an alias of a new `UnstractError` base, with `APIDeploymentError` and `PlatformClientError` beneath it. Catching the old name still catches both clients, including anything added later. Also here: - The generated models are re-exported, so callers who want typing can `WhoAmIResponse.from_dict(...)` instead of us hand-writing a mirror of the spec that regeneration would not update. Facade methods keep returning `dict[str, Any]`. - The platform client gets its own logger. Both clients shared the module logger, so levelling one re-levelled the other, switching a live sibling's debug output -- which includes response bodies -- on or off as a side effect. - A 2xx body that is unreadable, or JSON that is not an object, is now an error naming what arrived rather than an `AttributeError` downstream. The ERROR log for it is bounded to the same excerpt the exception carries. - An `org_id` that is empty or blank is refused before the request, and a path on `base_url` is warned about rather than silently discarded by `urljoin`. - `.claude/skills/spec-upgrade/SKILL.md` step 5 gains the recipe for adding an operation: which class it belongs to, the method shape, and why it builds from `_get_kwargs` rather than `sync_detailed`. No runtime breaking change. The one visible shift is `type(e).__name__`, which becomes `APIDeploymentError` where it was `APIDeploymentsClientException`; `except APIDeploymentsClientException` is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnhmFYFRBrK376tZyDtwUM
chandrasekharan-zipstack
added a commit
that referenced
this pull request
Sep 10, 2026
PR #29 gave the platform client its own copy of the transport: the pool, the close/reopen handling and the httpx-to-requests exception translation were duplicated from APIDeploymentsClient, and the copy had no retry policy at all. Two copies of that code drift; only one of them was getting fixes. Both clients now inherit `_HttpxFacade`, which owns the lazily built pooled transport, `close`/context-manager support, the exception translation and the retry-with-Retry-After policy. A client subclass supplies its error class and its methods, nothing else. That drops ~184 duplicated lines and gives the platform operations the retry behaviour the README already promised. Three defects fall out of sharing the code: - The transport pool is now built inside the lock. `AuthenticatedClient .get_httpx_client()` builds lazily and unsynchronised, so publishing the client before warming it let two threads build two pools. - A close during flight no longer escapes untranslated. httpx answers a send on a closed client with a bare `RuntimeError`, which is not in the subtree `_translate_transport_errors` covers, so it reached callers catching the documented `requests` types. It is translated at the send. - `list_deployments` no longer sends `workflow=None`. The generated builder renders that parameter with `str()` before it filters `None` out, so the literal string "None" went on the wire as a filter matching no workflow on every otherwise unfiltered call. Unset filters are omitted instead, which also holds if the generator special-cases another parameter later. Exceptions get a hierarchy. `APIDeploymentsClientException` never worked -- its `__init__` nested three more `def`s that were never bound to the class, so `message` was dropped and `Exception.__init__` was never called, leaving `str(e)` empty and the documented `error_message()` non-existent. It is now an alias of a new `UnstractError` base, with `APIDeploymentError` and `PlatformClientError` beneath it. Catching the old name still catches both clients, including anything added later. Also here: - The generated models are re-exported, so callers who want typing can `WhoAmIResponse.from_dict(...)` instead of us hand-writing a mirror of the spec that regeneration would not update. Facade methods keep returning `dict[str, Any]`. - The platform client gets its own logger. Both clients shared the module logger, so levelling one re-levelled the other, switching a live sibling's debug output -- which includes response bodies -- on or off as a side effect. - A 2xx body that is unreadable, or JSON that is not an object, is now an error naming what arrived rather than an `AttributeError` downstream. The ERROR log for it is bounded to the same excerpt the exception carries. - An `org_id` that is empty or blank is refused before the request, and a path on `base_url` is warned about rather than silently discarded by `urljoin`. - `.claude/skills/spec-upgrade/SKILL.md` step 5 gains the recipe for adding an operation: which class it belongs to, the method shape, and why it builds from `_get_kwargs` rather than `sync_detailed`. No runtime breaking change. The one visible shift is `type(e).__name__`, which becomes `APIDeploymentError` where it was `APIDeploymentsClientException`; `except APIDeploymentsClientException` is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CnhmFYFRBrK376tZyDtwUM
PR #29 gave the platform client its own copy of the transport: the pool, the close/reopen handling and the httpx-to-requests exception translation were duplicated from APIDeploymentsClient, and the copy had no retry policy at all. Two copies of that code drift; only one of them was getting fixes. Both clients now inherit `_HttpxFacade`, which owns the lazily built pooled transport, `close`/context-manager support, the exception translation and the retry-with-Retry-After policy. A client subclass supplies its error class and its methods, nothing else. That drops ~184 duplicated lines and gives the platform operations the retry behaviour the README already promised. Three defects fall out of sharing the code: - The transport pool is now built inside the lock. `AuthenticatedClient .get_httpx_client()` builds lazily and unsynchronised, so publishing the client before warming it let two threads build two pools. - A close during flight no longer escapes untranslated. httpx answers a send on a closed client with a bare `RuntimeError`, which is not in the subtree `_translate_transport_errors` covers, so it reached callers catching the documented `requests` types. It is translated at the send. - `list_deployments` no longer sends `workflow=None`. The generated builder renders that parameter with `str()` before it filters `None` out, so the literal string "None" went on the wire as a filter matching no workflow on every otherwise unfiltered call. Unset filters are omitted instead, which also holds if the generator special-cases another parameter later. Exceptions get a hierarchy. `APIDeploymentsClientException` never worked -- its `__init__` nested three more `def`s that were never bound to the class, so `message` was dropped and `Exception.__init__` was never called, leaving `str(e)` empty and the documented `error_message()` non-existent. It is now an alias of a new `UnstractError` base, with `APIDeploymentError` and `PlatformClientError` beneath it. Catching the old name still catches both clients, including anything added later. Also here: - The generated models are re-exported, so callers who want typing can `WhoAmIResponse.from_dict(...)` instead of us hand-writing a mirror of the spec that regeneration would not update. Facade methods keep returning `dict[str, Any]`. - The platform client gets its own logger. Both clients shared the module logger, so levelling one re-levelled the other, switching a live sibling's debug output -- which includes response bodies -- on or off as a side effect. - A 2xx body that is unreadable, or JSON that is not an object, is now an error naming what arrived rather than an `AttributeError` downstream. The ERROR log for it is bounded to the same excerpt the exception carries. - An `org_id` that is empty or blank is refused before the request, and a path on `base_url` is warned about rather than silently discarded by `urljoin`. - `.claude/skills/spec-upgrade/SKILL.md` step 5 gains the recipe for adding an operation: which class it belongs to, the method shape, and why it builds from `_get_kwargs` rather than `sync_detailed`. No runtime breaking change. The one visible shift is `type(e).__name__`, which becomes `APIDeploymentError` where it was `APIDeploymentsClientException`; `except APIDeploymentsClientException` is unaffected. Claude-Session: https://claude.ai/code/session_01CnhmFYFRBrK376tZyDtwUM Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
chandrasekharan-zipstack
approved these changes
Sep 10, 2026
chandrasekharan-zipstack
deleted the
feat/whoami-and-deployment-listing
branch
September 10, 2026 12:58
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
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.
What
Runs the
spec-upgradepipeline againstZipstack/unstractmain at520b98d7a, picking up the two operations the CLI needs:whoami/api/v1/unstract/whoami/list_deployments/api/v1/unstract/{org_id}/api/deployment/From Zipstack/unstract#2269 and Zipstack/unstract#2278.
Why
unstract-clicurrently reaches intounstract.clone.PlatformClient— the org-cloning tool's hand-written admin client — to do org discovery and deployment listing, because neither operation existed on the generated surface. That is how the CLI drifted off the spec pipeline: its vendoreddocstudio.jsonis still from0c5f36dab, andtests/test_contract.pyis structurally blind to commands that don't derive from a spec.This makes both operations reachable from the SDK, so the CLI can drop its
CLIPlatformClientsubclass and a customer can do the same work from either surface.How
Steps 1–2 — spec and provenance move together. Copied byte-for-byte;
SPEC_SOURCEgains the new revision and sha256 in this same commit. I verified the previous record first — the vendored file matched its recorded sha256 and was byte-identical to the backend ateddd4b746— so this starts from an honest baseline rather than assuming one.Steps 3–4 — regeneration is purely additive. Reviewed with
git add -Nso newly created files were visible:api/identity/whoami.py, newapi/deployment/list_deployments.pyWhoAmIResponse,PlatformKeyError,ApiKeyPermission,APIDeploymentSummary,PaginatedAPIDeploymentSummaryList, and a run-statuses item)execute.py/status.pychanged — prose only, where #2278 reworded descriptions. I filtered the diff to non-docstring lines to confirm nothing functional moved.No operation, field or model removed ⇒ minor, not major. The generator exited clean, and a second regeneration is byte-identical, so
sdk-driftwill pass.Step 5 —
PlatformAPIClient, a sibling rather than an extension.APIDeploymentsClienttakes a deployment URL and derives org + api_name from its last two segments;whoamihas neither, and both new operations take a platform key rather than a deployment key. So the new class shares the generated transport (both schemes are HTTP bearer — only the token differs) and the same exception type, but owns its own construction. Folding them together would have meant a class whose requiredapi_urlis meaningless for half its methods.Step 6 — 438 tests pass, up from 419.
__version__and the compat baseline are deliberately untouched, per the skill: the release workflow reads the former as the last released version and bumps at dispatch, and a spec upgrade is not a reason to move the parity reference point.Review round: six findings, all fixed in
3e9717eA
/code-reviewpass at medium effort found six issues in the first commit's facade. Worth recording that the same review at low effort reported the PR clean — the medium pass is what surfaced these.sync_detailedreaches_parse_response, which doesPlatformKeyError.from_dict(response.json())on a 401 with no guard: a gateway answering 401 with HTML raisesJSONDecodeError, a DRF-shaped{"detail": …}raisesKeyError: 'message'— both before the facade sees the response. Now built from the generated_get_kwargsand read viaAPIDeploymentsClient._read_body, which is why that helper exists_send, so transport failures arrive as therequeststypes the module docstring promises, rather than rawhttpx.ConnectErrorclose()/__enter__/__exit__— pooled connections had nothing to release themAuthenticatedClientbakes its own on first use, so a reassignedapi_keywas silently ignoredFindings 1–2 are the same defect class as the one below: the first commit fixed the half where reporting a refusal crashed, and missed the half where building the model crashed first.
Five mutations killed to confirm the fixes are pinned: unguarded
response.json(), dropping the transport translation, capturing the key with the transport, neuteringclose, and removing the re-export.A defect the first commit's tests caught
_error_textfell back toresponse.text, but the generatedResponseis an attrs wrapper carrying.content, andparsedis a model instance rather than a mapping. A 401 through the new client would have raisedAttributeErrorwhile trying to report the refusal — a crash instead of "your key was rejected". Both halves are fixed; the_error_textchange is additive (getattr(response, "text", None), then decode.content), so httpx callers behave identically.Two decisions I'd like a second opinion on
1. The class name.
PlatformClientis already taken byunstract.clone, and two classes of that name shipping in one distribution seemed worse than a slightly longer one.PlatformAPIClientis my choice, not a considered team convention — easy to rename before release.2. The test manifest shape.
test_every_declared_operation_is_wrappedfired exactly as designed when the spec grew. ExtendingWRAPPED_OPERATIONSto four was not viable, though:whoamideclares noErrorResponse, so that suite'sassert set(errors.values()) > {"ErrorResponse"}is false for it by construction;_declared_responsesindexescontentunconditionally, and both new operations declare a bodyless 500, which wouldKeyError.So there is a parallel
PLATFORM_OPERATIONSmanifest with its own status pins and error-reporting coverage, and the whole-set comparison unions the two — preserving the property that test exists for (an operation belonging to neither still fails) without forcing platform operations through assertions that are untrue for them. If you'd rather the two families converge, that's a bigger change to the existing assertions and I'd want your call first.Can this PR break any existing features
No.
APIDeploymentsClientis untouched apart from the additive_error_textfallback (its_read_bodyis now also reused by the new class);executeandstatuschanged only in docstrings. The 419 pre-existing tests all still pass.Notes on Testing
438 passing. New coverage: both operations' declared error statuses reported with their reason, the four whoami fields, the org segment and query params reaching
list_deployments, the paginated envelope read back, key-from-environment fallback, and refusals at construction for a missing key or a hostless base URL. Patched atAuthenticatedClient.get_httpx_clientrather than abovesync_detailed, so the generated parsing and model construction are genuinely exercised — that is the layer a regeneration changes.Pre-existing lint debt in
src/unstract/clone/**andtests/clone/**is untouched and unrelated.Next
Not in this PR: a minor release to PyPI, then
bump-client-pinsinunstract-cliand deletingCLIPlatformClient.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ