Skip to content

feat: pick up the whoami and deployment-listing operations - #29

Merged
chandrasekharan-zipstack merged 4 commits into
mainfrom
feat/whoami-and-deployment-listing
Sep 10, 2026
Merged

feat: pick up the whoami and deployment-listing operations#29
chandrasekharan-zipstack merged 4 commits into
mainfrom
feat/whoami-and-deployment-listing

Conversation

@praveen-formido

@praveen-formido praveen-formido commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What

Runs the spec-upgrade pipeline against Zipstack/unstract main at 520b98d7a, picking up the two operations the CLI needs:

operation path credential
whoami /api/v1/unstract/whoami/ platform key
list_deployments /api/v1/unstract/{org_id}/api/deployment/ platform key

From Zipstack/unstract#2269 and Zipstack/unstract#2278.

Why

unstract-cli currently reaches into unstract.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 vendored docstudio.json is still from 0c5f36dab, and tests/test_contract.py is 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 CLIPlatformClient subclass 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_SOURCE gains 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 at eddd4b746 — so this starts from an honest baseline rather than assuming one.

Steps 3–4 — regeneration is purely additive. Reviewed with git add -N so newly created files were visible:

  • new api/identity/whoami.py, new api/deployment/list_deployments.py
  • six new models (WhoAmIResponse, PlatformKeyError, ApiKeyPermission, APIDeploymentSummary, PaginatedAPIDeploymentSummaryList, and a run-statuses item)
  • execute.py / status.py changed — 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-drift will pass.

Step 5 — PlatformAPIClient, a sibling rather than an extension. APIDeploymentsClient takes a deployment URL and derives org + api_name from its last two segments; whoami has 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 required api_url is 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 3e9717e

A /code-review pass 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.

# Fix
1–2 The body is read as JSON, not through the generated model. 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, a DRF-shaped {"detail": …} raises KeyError: 'message' — both before the facade sees the response. Now built from the generated _get_kwargs and read via APIDeploymentsClient._read_body, which is why that helper exists
3 Requests go through _send, so transport failures arrive as the requests types the module docstring promises, rather than raw httpx.ConnectError
4 close() / __enter__ / __exit__ — pooled connections had nothing to release them
5 Auth header set per request; AuthenticatedClient bakes its own on first use, so a reassigned api_key was silently ignored
6 Re-exported from the package root

Findings 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, neutering close, and removing the re-export.

A defect the first commit's 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 — a crash instead of "your key was rejected". Both halves are fixed; the _error_text change 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. PlatformClient is already taken by unstract.clone, and two classes of that name shipping in one distribution seemed worse than a slightly longer one. PlatformAPIClient is my choice, not a considered team convention — easy to rename before release.

2. The test manifest shape. test_every_declared_operation_is_wrapped fired exactly as designed when the spec grew. Extending WRAPPED_OPERATIONS to four was not viable, though:

  • whoami declares no ErrorResponse, so that suite's assert set(errors.values()) > {"ErrorResponse"} is false for it by construction;
  • _declared_responses indexes content unconditionally, and both new operations declare a bodyless 500, which would KeyError.

So there is a parallel PLATFORM_OPERATIONS manifest 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. APIDeploymentsClient is untouched apart from the additive _error_text fallback (its _read_body is now also reused by the new class); execute and status changed 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 at AuthenticatedClient.get_httpx_client rather than above sync_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/** and tests/clone/** is untouched and unrelated.

Next

Not in this PR: a minor release to PyPI, then bump-client-pins in unstract-cli and deleting CLIPlatformClient.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y7U4ggFchu91zKYcRxFRNZ

praveen-formido and others added 2 commits September 10, 2026 15:04
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
praveen-formido marked this pull request as ready for review September 10, 2026 11:22
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

via Greptile

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the previously reported generated-parser coverage gap is resolved and no new actionable defect remains.

Summary

  • Adds generated whoami and paginated deployment-listing endpoints and response models.
  • Introduces PlatformKeyClient with retries, transport error translation, connection lifecycle management, and environment-based credential fallback.
  • Refactors deployment and platform clients onto a shared transport facade.
  • Adds package-level exports, usage documentation, and compatibility coverage for generated parsing, request construction, errors, retries, and concurrency.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Caller --> PKC[PlatformKeyClient]
    PKC --> Shared[_HttpxFacade]
    Shared --> WhoAmI[GET /api/v1/unstract/whoami/]
    WhoAmI --> Org[organization_id]
    Org --> Listing[GET /api/v1/unstract/org_id/api/deployment/]
    Listing --> Page[Paginated deployment results]
    Caller --> ADC[APIDeploymentsClient]
    ADC --> Shared
    Shared --> Transport[Authenticated httpx transport]
Loading

Reviews (3) · Last reviewed commit: "refactor: fold the platform client into ..."

Comment thread tests/test_compat.py Outdated
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
chandrasekharan-zipstack merged commit c98792d into main Sep 10, 2026
4 checks passed
@chandrasekharan-zipstack
chandrasekharan-zipstack deleted the feat/whoami-and-deployment-listing branch September 10, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants