Skip to content

SDK 0.2.0: two-client architecture (UserClient + AgentClient)#3

Merged
AlexLiu190625 merged 16 commits into
xorbitsai:mainfrom
AlexLiu190625:feat/0.2.0-user-client
May 31, 2026
Merged

SDK 0.2.0: two-client architecture (UserClient + AgentClient)#3
AlexLiu190625 merged 16 commits into
xorbitsai:mainfrom
AlexLiu190625:feat/0.2.0-user-client

Conversation

@AlexLiu190625

@AlexLiu190625 AlexLiu190625 commented May 30, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Breaking change

0.1.0 code breaks on upgrade. from xagent_sdk import XAgentClient and
MeResponse now raise ImportError. This is intentional — there is no
deprecation alias. See the migration table below.

What this does

Splits the single XAgentClient into two clients over a shared
_BaseClient, matching the two key types the backend issues:

  • UserClient — authenticates with a personal key
    (xag_personal_<prefix>_<secret>). Owns the management surface:
    me(), templates.{list,get}, agents.{list,create,create_from_template,rotate_key}.
  • AgentClient — authenticates with an agent runtime key
    (xag_<prefix>_<secret>). Owns tasks.{create,append,get,steps,wait,run}.

A SaaS app uses UserClient to mint an agent + one-time runtime key,
then hands the runtime key to AgentClient to drive chat tasks.

Migration from 0.1.0

0.1.0 0.2.0
from xagent_sdk import XAgentClient from xagent_sdk import AgentClient (tasks) / UserClient (management)
client.me()MeResponse(agent_id, ...) UserClient.me()UserPrincipal(user_id, ...)
client.tasks.* AgentClient.tasks.* (unchanged surface)

sed -i '' 's/XAgentClient/AgentClient/g' your_app.py covers the rename;
identity lookups move to UserClient.me() with a different field shape.

New public surface

  • Clients: UserClient, AgentClient
  • Dataclasses: UserPrincipal, Template, TemplateDetail,
    AgentSummary, AgentCreateResult, RotateKeyResult
  • Errors: TemplateNotFound (server code template_not_found),
    MalformedResponse (SDK-coined, for decode-shape failures)

Validation

  • 131 unit tests pass; mypy strict + ruff + codespell clean.
  • Public surface pinned to an exact 28-name set; a re-export of the old
    names trips CI (test_public_surface.py + a repo-wide grep guard).
  • Every SDK method exercised against a live backend: me, templates
    list/get, agents list/create/create_from_template (both
    generate_runtime_key modes)/rotate_key, tasks
    create/get/steps/append, and the 404 → typed-exception mappings
    (TemplateNotFound / AgentNotFound / TaskNotFound,
    revoked-key → InvalidAPIKey).
  • Response parsers were aligned to the real backend wire shapes
    (bare-array list endpoints, idtemplate_id/agent_id, nested
    {agent, api_key} create payload, flat-spread template overrides).

Hoist the env-variable resolution, HTTPClient ownership, _request
helper (4xx/5xx -> XAgentError mapping), close(), and the
context-manager protocol out of XAgentClient into a new
_BaseClient base class in xagent_sdk/_base.py. XAgentClient now
inherits from it and only declares its own __init__ glue
(super().__init__ + self.tasks = TasksAPI(self)) and the public
me() probe.

The base class customizes per subclass via two ClassVars
(_ENV_API_KEY, _API_KEY_FIELD), so the upcoming UserClient can
reuse the same plumbing while reading XAGENT_PERSONAL_KEY and
showing a tailored ValueError when the key is missing.

No public behavior change; all 85 unit tests still pass.
0.2.0 splits the SDK into two public clients: AgentClient for runtime
chat-task calls (this commit) and UserClient for management endpoints
(landing in a later commit). The runtime client used to be called
XAgentClient and exposed a me() probe that returned the agent
identity bound to the runtime key; in the new model identity lives
on the personal key and is read via UserClient.me(), so AgentClient
loses me() entirely.

Changes in this commit:
- git mv src/xagent_sdk/client.py -> agent_client.py and rename
  the class XAgentClient -> AgentClient; the class no longer
  imports MeResponse / _parse_me and no longer has a me() method.
- Update tasks.py forward-ref ("XAgentClient" -> "AgentClient") and
  the TasksAPI docstring reference accordingly.
- Update __init__.py to re-export AgentClient instead of
  XAgentClient (MeResponse re-export stays until Phase F).
- git mv tests/unit/test_client.py -> test_agent_client.py;
  delete the TestMe class (the two me() tests are subsumed by
  test_errors.py's parametrize for 401 -> InvalidAPIKey and by
  the env-var fallback assertions which now inspect the
  Authorization header directly).
- Batch-rename XAgentClient -> AgentClient in tests/unit/conftest.py,
  tests/unit/test_tasks.py, tests/e2e/conftest.py, and
  tests/e2e/test_smoke.py. The e2e test_smoke still references
  client.me(); that file is rewritten in Phase G when the
  two-step personal+runtime flow lands.
- clean_xagent_env autouse now also clears XAGENT_PERSONAL_KEY so
  UserClient tests landing next phases stay isolated.

No public behavior change for the surviving runtime surface; 83 unit
tests pass (down from 85: deleted two me() tests).
…NotFound

Land the dataclass surface and error mapping the upcoming UserClient
needs. The 0.1.0 MeResponse class is removed in the same change to
avoid leaving a stub that would import-resolve but no longer matches
the /v1/me response shape (which now returns a user principal rather
than an agent identity).

New dataclasses in types.py:
- UserPrincipal -- /v1/me response. Replaces MeResponse.
- Template / TemplateDetail -- /v1/templates list entry + detail
  (the detail variant adds agent_config, the merge target for
  create_from_template overrides).
- AgentSummary -- /v1/agents list entry.
- AgentCreateResult -- /v1/agents and /v1/agents/from-template
  response, carries the one-time runtime_full_key when the backend
  default generate_runtime_key=True path is taken.
- RotateKeyResult -- /v1/agents/{id}/api-key response. The method
  name documents the rotation side-effect; the dataclass exposes the
  one-time full_key plus the public-safe key_prefix.

Six new private TypeAdapter helpers (_parse_user_principal /
_parse_template_list / _parse_template_detail / _parse_agent_list /
_parse_agent_create / _parse_rotate_key) follow the existing
pydantic v2 pattern used by _parse_task_info etc. The list parsers
defensively short-circuit to [] on non-dict input, mirroring
_parse_steps.

New error class TemplateNotFound (404 code template_not_found),
registered in errors._CODE_MAP. Distinct from AgentNotFound because
the SaaS template-picker UX treats the two mismatches differently.

Public surface adjustments in this commit are deliberately minimal:
remove MeResponse from xagent_sdk/__init__.py + tests/unit/test_types.py
so the package still imports, but defer the rest of the surface
shuffle (adding the seven new names, asserting the expected __all__)
to Phase F. Test rename test_me_frozen -> test_step_frozen keeps the
frozen-dataclass invariant covered.

82 unit tests pass.
Land the personal-key management client and its two namespaces.

src/xagent_sdk/user_client.py:
- UserClient extends _BaseClient with _ENV_API_KEY=XAGENT_PERSONAL_KEY
  and _API_KEY_FIELD=personal_key, so its constructor reads
  personal_key from the kwarg or env and the missing-key ValueError
  message correctly names the personal_key parameter.
- me() returns the new UserPrincipal (user_id / email / name /
  principal_type / key_prefix) parsed via _parse_user_principal.
- Composes .templates = TemplatesAPI(self) and
  .agents = AgentsAPI(self) at construction time, following the same
  pattern AgentClient uses for .tasks.

src/xagent_sdk/_templates.py:
- TemplatesAPI.list() -> list[Template] hits GET /v1/templates and
  defensively returns [] for non-dict bodies (mirrors _parse_steps).
- TemplatesAPI.get(template_id) -> TemplateDetail hits
  GET /v1/templates/{template_id}; backend 404 with template_not_found
  surfaces as TemplateNotFound via the V1 envelope mapping wired in
  the previous commit.

src/xagent_sdk/_agents.py:
- AgentsAPI.list() -> list[AgentSummary]: GET /v1/agents.
- AgentsAPI.create(name, instructions, generate_runtime_key=True,
  metadata=None): POST /v1/agents. metadata is omitted from the wire
  when None, matching tasks.create's convention. The returned
  AgentCreateResult carries runtime_full_key as a one-time payload
  when generate_runtime_key=True; docstring warns to vault and never
  log it.
- AgentsAPI.create_from_template(template_id, overrides=None,
  generate_runtime_key=True): POST /v1/agents/from-template. overrides
  accepts a Mapping[str, Any] and is converted to a dict before
  serialization; SDK does no client-side schema validation -- backend
  422 invalid_input on malformed overrides.
- AgentsAPI.rotate_key(agent_id): POST /v1/agents/{id}/api-key. The
  method name encodes the destructive side effect (revoke + insert
  atomically); docstring spells out that existing AgentClient
  instances using the old key will start receiving InvalidAPIKey on
  the next request.

Both namespace classes follow the existing TasksAPI pattern: hold a
reference to the owning client and route every call through
self._client._request, then dispatch to a _parse_* helper on the
success body.

No tests for the new classes in this commit -- fixtures and unit
tests land in Phase E. 82 unit tests still pass.
Cover the 0.2.0 management surface with hermetic unit tests and the
canonical wire fixtures other language clients will reuse.

shared/fixtures/v1/responses/:
- DELETE the 0.1.0 me.json (agent identity shape) and replace with
  me_user.json (UserPrincipal shape: principal_type / user_id /
  email / name / key_prefix).
- NEW templates_list.json (slim wrapper {templates: [...]} covering
  the four Phase 2 default templates: Content Generator, Analyzer,
  Q&A, Assistant).
- NEW templates_detail.json (full agent_config dict, the merge
  target for create_from_template overrides).
- NEW agents_list.json (covers all three documented status values:
  active / draft / paused).
- NEW agents_create.json (default generate_runtime_key=True path
  carrying the one-time runtime_full_key + the public runtime_key_prefix).
- NEW rotate_key.json (rotation result with one-time full_key,
  public key_prefix, and timestamp).

shared/fixtures/v1/errors/:
- NEW template_not_found.json (V1 envelope; new stable code in this
  release).

shared/README.md: extend both fixture tables and call out which
fixture each method consumes plus the 0.2.0+ marker on the new entries.

python/tests/unit/:
- test_user_client.py: construction (explicit / XAGENT_PERSONAL_KEY
  env fallback / explicit-overrides-env / missing key / missing
  base_url), invariant that UserClient does NOT silently fall back to
  XAGENT_API_KEY (that env belongs to AgentClient), me() returns
  UserPrincipal with the canonical fixture values, 401 -> InvalidAPIKey,
  and context-manager close.
- test_templates.py: list() URL + parse + empty list + defensive
  non-dict body; get() URL + parse + 404 -> TemplateNotFound.
- test_agents_management.py: list() URL + parse + empty list;
  create() default-body shape (generate_runtime_key=True wire),
  generate_runtime_key=False pass-through, metadata included /
  omitted on None, 422 -> InvalidInput; create_from_template body
  shape with overrides, overrides omitted when None,
  template_not_found mapping; rotate_key() URL + parse, 404 ->
  AgentNotFound.
- test_clients_isolated.py: two clients in the same process produce
  distinct Authorization headers, distinct httpx.Client instances,
  and closing one does not close the other.
- test_errors.py: extend the stable-code parametrize with
  (404, "template_not_found", TemplateNotFound) so the V1 envelope
  mapping is fixture-driven for the new code too.

python/src/xagent_sdk/__init__.py: re-export the seven new public
symbols (UserClient, UserPrincipal, Template, TemplateDetail,
AgentSummary, AgentCreateResult, RotateKeyResult, TemplateNotFound)
so the new tests can `from xagent_sdk import ...`. Final __all__
ordering and the test_public_surface assertion land in Phase F
alongside the version bump.

111 unit tests pass.
Mark the breaking release with the version bump and add two
mechanical pins so the 0.1.0 -> 0.2.0 rename cannot silently regress.

Version files:
- python/src/xagent_sdk/_version.py: __version__ = "0.2.0".
- python/pyproject.toml: project version = "0.2.0".

The version also surfaces in the User-Agent header
(``xagent-sdk-python/0.2.0 (httpx/...)``) so the backend can
correlate traffic to release.

python/tests/unit/test_public_surface.py:
- EXPECTED_SURFACE: the canonical 27-name 0.2.0 __all__ set.
- test_all_matches_expected: equality (not subset) check so future
  additions must justify themselves explicitly.
- test_every_exported_name_resolves: every name in __all__ is also
  importable on the package (catches forgotten imports).
- test_xagent_client_legacy_name_removed: hasattr + __all__ check
  for the renamed runtime client.
- test_meresponse_legacy_name_removed: same for the replaced
  response dataclass.
- test_version_bumped_to_0_2_0: pin the announced version string.

python/tests/unit/test_check_no_legacy_callsites.py:
- Three subprocess greps that scan python/src + python/tests for the
  legacy runtime-client name, MeResponse, and the _parse_me helper.
  Excludes this test file plus test_public_surface (the only places
  that intentionally reference the legacy patterns by design via
  string concatenation). Documentation files are out of scope here
  -- python/README.md ships its full rewrite in the next commit.

Source-side cleanup pulled into this commit so the greps land green:
- _base.py and _http.py docstrings referenced XAgentClient by name
  in narrative text that did not get updated when Phase B renamed
  the class. Rewritten to mention AgentClient / UserClient.
- types.py::UserPrincipal docstring used to point readers at the
  legacy MeResponse class name; reworded to "the 0.1.0 agent-identity
  shape" so the grep test does not need to special-case it.

119 unit tests pass.
Land the 0.2.0 user-facing documentation and the e2e fixture split.
This is the last commit of the breaking-release series.

python/README.md (full rewrite):
- Status banner calls out the breaking change vs 0.1.0 and links
  forward to the migration section.
- Install command pinned to @v0.2.0 with #subdirectory=python.
- Two distinct env vars documented (XAGENT_PERSONAL_KEY for
  UserClient, XAGENT_API_KEY for AgentClient) with the rationale
  for keeping them separate.
- New "Migration from 0.1.0" section: which symbols moved, what the
  user-side sed pass looks like, and the fact that legacy imports
  fail loudly at startup rather than silently at first use.
- Quick start is a two-step happy path: UserClient.agents.create_from_template
  to mint an agent + runtime key, then AgentClient(api_key=runtime_key)
  to run it. Vault-warning on the one-time runtime_full_key.
- Concepts section maps the 0.2.0 vocabulary (User / Personal key /
  Agent / Runtime key / Template / Task / Step).
- Six examples cover identity probe, template-driven create, agent
  list, runtime-key rotation, multi-turn append, and error handling.
- API reference split into a UserClient table and an AgentClient
  table with their respective endpoint paths and notes.
- Configuration shows both constructors side by side and reaffirms
  the cross-client isolation invariant the unit tests pin.
- Development section: the e2e command now requires both env vars,
  E2E_AGENT_ID gates the runtime-only smoke tests, E2E_TEMPLATE_ID /
  E2E_AGENT_NAME drive the full-flow test.

python/tests/e2e/conftest.py (rewrite):
- Two paired fixture sets: user_client / patient_user_client (read
  XAGENT_PERSONAL_KEY) and agent_client / patient_agent_client
  (read XAGENT_API_KEY). Each pair's "patient_" variant raises the
  per-request HTTP timeout from 30s to 60s.
- pytest.skip() is split between management (needs personal key)
  and runtime (needs agent key) so a developer with only one of the
  two halves can still exercise the other.

python/tests/e2e/test_smoke.py (rewrite):
- test_user_me: 0.2.0 identity probe; asserts UserPrincipal shape
  (principal_type / user_id / email / name / key_prefix).
- test_create_is_async: same async-contract regression catcher as
  0.1.0, now bound to patient_agent_client and to E2E_AGENT_ID
  (since AgentClient lost its identity probe in 0.2.0, the test
  skips when the env var is missing rather than silently failing).
- test_run_single_turn: same gating; runtime-only.
- test_e2e_full_flow_phase2: the Phase 2 happy path end-to-end --
  list templates, create_from_template, build an AgentClient with
  the freshly minted runtime key, run a single-turn task, assert
  COMPLETED. Deliberately leaves the new agent in place since the
  SDK has no delete method; run against a scratch backend.

119 unit tests pass, 4 e2e tests deselected by default.
The 0.2.0 schema was built against an envelope-wrapped list shape
({"templates":[{template_id,name}]}, {"agents":[{agent_id,name}]}) and a
flat agent-create payload ({agent_id,name,runtime_full_key,...}).
Hitting the real backend revealed three mismatches that caused empty
lists and broken create flows:

- GET /v1/templates and GET /v1/agents return raw JSON arrays whose
  entries key the primary id under "id", not "template_id"/"agent_id".
- POST /v1/agents and POST /v1/agents/from-template return the new agent
  and the one-time runtime key under nested "agent" and "api_key"
  blocks, not flat at the top level.
- POST /v1/agents/from-template expects override fields spread flat at
  the request-body top level (V1AgentTemplateCreateRequest), not wrapped
  under an "overrides" key; the wrapped form was silently dropped, so
  the name override never took effect and create_from_template always
  reused the template's default name.

Parsers in types.py now rename id -> template_id/agent_id, unwrap the
nested create payload, and accept a bare list. AgentsAPI.create_from_template
spreads overrides flat so backend pydantic accepts the keys. The five
fixtures under shared/fixtures/v1/responses/ are rewritten to mirror the
real wire format observed against localhost:8000, and the affected unit
tests now pin the new shape (including a defensive "non-list body
returns []" pin to mirror _parse_steps semantics).

Public dataclass field names (template_id, agent_id, runtime_full_key,
...) and AgentsAPI.create_from_template's overrides=Mapping signature
are unchanged.
…ip process-history wording

Three changes bundled because they share the same review pass.

Parser robustness
- _parse_agent_create now raises XAgentTransportError("malformed_response", ...)
  when the response body lacks a usable "agent" block. The previous path let
  pydantic surface "Input should be a valid integer, input_value=None" on
  agent_id, which did not point at the real cause (backend response shape
  violation).
- _template_dict / _agent_summary_dict fall back to a pre-existing
  "template_id" / "agent_id" when the backend omits the canonical "id" field,
  so a future backend rename does not silently degrade the SDK surface to
  None.

Test coverage
- New TestParseAgentCreateMalformed in test_types.py pins the four
  malformed-body shapes that must surface the new error code.
- New TestNormalizeHelpers in test_types.py pins the id / fallback /
  collision behavior of the two rename helpers.

Wording cleanup
- Remove the "(v0.3.0+) Hardcoded production default URL -- not yet baked in"
  block from AgentClient / UserClient constructor docstrings: anchoring code
  to a future release name is process-history wording, not a current
  invariant.
- Drop release-history paragraphs from UserPrincipal docstring,
  test_public_surface.py, test_check_no_legacy_callsites.py, and
  test_agent_client.py. The mechanical pins (forbidden symbol grep, exact
  __all__ set) keep their force without narrating how the codebase got
  here.
- Trim "Reserved; backend does not yet emit" from RateLimited and
  "in early Phase 2 wire shapes" from AgentSummary.

131 unit tests pass; the four real-backend sanity calls (me / templates /
agents / runtime key issue) still succeed against localhost:8000.
_parse_agent_create raised XAgentTransportError("malformed_response",
...) when the response body lacked its required 'agent' block. That
overloaded the transport error -- the HTTP exchange had in fact
succeeded; only the decoded payload was wrong. Introduce a dedicated
MalformedResponse(XAgentError) so the exception type matches the failure
class: the body did not match the shape the SDK needs to build a result.

MalformedResponse is SDK-coined (the server never emits the code) so it
stays out of _CODE_MAP but joins the public __all__ so callers can catch
it. Public surface grows to 28 names; the README error table and the
surface pin are updated accordingly.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Python SDK to version 0.2.0, introducing breaking changes that split the legacy XAgentClient into two isolated clients: UserClient for workspace management (templates, agents, and user identity) and AgentClient for runtime chat tasks. It also adds support for template management, agent creation, and API key rotation. The review feedback focuses on enhancing the robustness of the SDK's response parsing helpers in types.py (_parse_user_principal, _parse_template_detail, _parse_agent_create, and _parse_rotate_key) by defensively validating that the backend responses are dictionaries and raising a clean MalformedResponse exception rather than letting raw Pydantic validation or type errors propagate.

Comment thread python/src/xagent_sdk/types.py Outdated
Comment on lines +265 to +266
def _parse_user_principal(data: dict[str, Any]) -> UserPrincipal:
return _USER_PRINCIPAL_ADAPTER.validate_python(data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _parse_user_principal function expects data to be a dictionary. If the backend returns a non-dictionary response, this will raise a raw Pydantic ValidationError. To align with the SDK's error handling strategy of raising MalformedResponse for response contract violations, we should defensively check that data is a dictionary.

def _parse_user_principal(data: Any) -> UserPrincipal:
    if not isinstance(data, dict):
        raise MalformedResponse(
            "malformed_response",
            "Expected a dictionary response for user principal",
            http_status=None,
        )
    return _USER_PRINCIPAL_ADAPTER.validate_python(data)

Comment thread python/src/xagent_sdk/types.py Outdated
Comment on lines +284 to +285
def _parse_template_detail(data: dict[str, Any]) -> TemplateDetail:
return _TEMPLATE_DETAIL_ADAPTER.validate_python(_template_dict(data))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _parse_template_detail function expects data to be a dictionary and passes it directly to _template_dict, which performs attribute accesses. If the backend returns a non-dictionary response, this will raise an unhandled AttributeError or TypeError. We should defensively check that data is a dictionary and raise a MalformedResponse if it is not.

def _parse_template_detail(data: Any) -> TemplateDetail:
    if not isinstance(data, dict):
        raise MalformedResponse(
            "malformed_response",
            "Expected a dictionary response for template detail",
            http_status=None,
        )
    return _TEMPLATE_DETAIL_ADAPTER.validate_python(_template_dict(data))

Comment thread python/src/xagent_sdk/types.py Outdated
Comment on lines +300 to +335
def _parse_agent_create(data: dict[str, Any]) -> AgentCreateResult:
"""Parse the nested ``{"agent": {...}, "api_key": {...}}`` payload.

Backend returns the new agent row and (when ``generate_runtime_key``
is true) the one-time runtime key as two siblings under separate
keys; SDK flattens them into ``AgentCreateResult`` so callers see one
record. When ``generate_runtime_key=False`` the ``api_key`` block is
absent and the runtime fields stay ``None`` -- caller is expected to
materialize a key via ``rotate_key()`` later.

Raises ``MalformedResponse`` if the ``agent`` block is missing or
lacks ``id``/``name`` -- pydantic's raw ``ValidationError`` on
``agent_id`` would say "Input should be a valid integer,
input_value=None" which does not point at the real cause (backend
response shape violation).
"""
agent = data.get("agent")
if (
not isinstance(agent, dict)
or agent.get("id") is None
or agent.get("name") is None
):
raise MalformedResponse(
"malformed_response",
"agent-create response missing required 'agent' block "
"(expected {'agent': {'id': int, 'name': str, ...}, 'api_key'?: {...}})",
http_status=None,
)
api_key = data.get("api_key") or {}
flat = {
"agent_id": agent.get("id"),
"name": agent.get("name"),
"runtime_full_key": api_key.get("full_key"),
"runtime_key_prefix": api_key.get("key_prefix"),
}
return _AGENT_CREATE_ADAPTER.validate_python(flat)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _parse_agent_create function performs dictionary lookups and attribute accesses on data and api_key without verifying their types first. If the backend returns a non-dictionary response or a non-dictionary api_key block, this will raise an unhandled AttributeError or TypeError instead of a clean MalformedResponse. We should defensively check that data is a dictionary and that api_key is a dictionary before performing lookups.

def _parse_agent_create(data: Any) -> AgentCreateResult:
    """Parse the nested ``{"agent": {...}, "api_key": {...}}`` payload.

    Backend returns the new agent row and (when ``generate_runtime_key``
    is true) the one-time runtime key as two siblings under separate
    keys; SDK flattens them into ``AgentCreateResult`` so callers see one
    record. When ``generate_runtime_key=False`` the ``api_key`` block is
    absent and the runtime fields stay ``None`` -- caller is expected to
    materialize a key via ``rotate_key()`` later.

    Raises ``MalformedResponse`` if the ``agent`` block is missing or
    lacks ``id``/``name`` -- pydantic's raw ``ValidationError`` on
    ``agent_id`` would say "Input should be a valid integer,
    input_value=None" which does not point at the real cause (backend
    response shape violation).
    """
    if not isinstance(data, dict):
        raise MalformedResponse(
            "malformed_response",
            "Expected a dictionary response for agent creation",
            http_status=None,
        )
    agent = data.get("agent")
    if (
        not isinstance(agent, dict)
        or agent.get("id") is None
        or agent.get("name") is None
    ):
        raise MalformedResponse(
            "malformed_response",
            "agent-create response missing required 'agent' block "
            "(expected {'agent': {'id': int, 'name': str, ...}, 'api_key'?: {...}})",
            http_status=None,
        )
    api_key = data.get("api_key")
    if not isinstance(api_key, dict):
        api_key = {}
    flat = {
        "agent_id": agent.get("id"),
        "name": agent.get("name"),
        "runtime_full_key": api_key.get("full_key"),
        "runtime_key_prefix": api_key.get("key_prefix"),
    }
    return _AGENT_CREATE_ADAPTER.validate_python(flat)

Comment thread python/src/xagent_sdk/types.py Outdated
Comment on lines +355 to +356
def _parse_rotate_key(data: dict[str, Any]) -> RotateKeyResult:
return _ROTATE_KEY_ADAPTER.validate_python(data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _parse_rotate_key function expects data to be a dictionary. If the backend returns a non-dictionary response, this will raise a raw Pydantic ValidationError. To align with the SDK's error handling strategy of raising MalformedResponse for response contract violations, we should defensively check that data is a dictionary.

Suggested change
def _parse_rotate_key(data: dict[str, Any]) -> RotateKeyResult:
return _ROTATE_KEY_ADAPTER.validate_python(data)
def _parse_rotate_key(data: Any) -> RotateKeyResult:
if not isinstance(data, dict):
raise MalformedResponse(
"malformed_response",
"Expected a dictionary response for key rotation",
http_status=None,
)
return _ROTATE_KEY_ADAPTER.validate_python(data)

The list parsers guard with isinstance(data, list); the single-object
parsers did not, so a backend (or proxy) returning a non-object body
surfaced as a raw pydantic ValidationError or AttributeError that did
not name the real cause. Add a shared _require_mapping(data, what) helper
and route every single-object parser through it -- _parse_user_principal,
_parse_template_detail, _parse_agent_create, _parse_rotate_key,
_parse_create_task, _parse_append, _parse_task_info -- so they all raise
MalformedResponse uniformly. _parse_agent_create additionally coerces a
non-dict api_key block to {} instead of trusting its .get().

Establishes the invariant: every parser validates its input shape before
trusting it. Covered by a parametrized matrix (7 parsers x 5 non-dict
bodies) plus the api_key-coercion case.
@AlexLiu190625

Copy link
Copy Markdown
Collaborator Author

Thanks — applied in 3bc7692. Rather than guarding the four flagged parsers individually, I extracted a shared _require_mapping(data, what) helper and routed every single-object parser through it (_parse_user_principal, _parse_template_detail, _parse_agent_create, _parse_rotate_key, plus the three runtime parsers _parse_create_task / _parse_append / _parse_task_info), so the whole parser layer now raises MalformedResponse uniformly on a non-object body. _parse_agent_create also coerces a non-dict api_key block to {} as suggested.

Covered by a parametrized matrix (7 parsers × 5 non-dict bodies) plus the api_key coercion case; full suite is 167 green.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two follow-up findings from review.

Comment thread python/src/xagent_sdk/_agents.py Outdated
if metadata is not None:
body["metadata"] = metadata
resp = self._client._request("POST", "/v1/agents", json=body)
return _parse_agent_create(resp.json())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create() defaults generate_runtime_key to True, so a successful default create response is expected to include the one-time runtime key. If the backend response omits api_key or sends a non-object value here, _parse_agent_create() currently returns runtime_full_key=None; constructing an AgentClient(api_key=result.runtime_full_key) can then fall back to XAGENT_API_KEY and silently use the wrong credential. Please make the default-key path fail closed, for example by requiring api_key.full_key when generate_runtime_key=True (and applying the same check to create_from_template()).

Comment thread shared/README.md Outdated
Comment on lines +20 to +22
| `templates_list.json` | `GET /v1/templates` | Wrapper `{templates: [Template]}`; slim list entries with `template_id`, `name`, optional `description` |
| `templates_detail.json` | `GET /v1/templates/{id}` | Single template with the merge-target `agent_config` dict |
| `agents_list.json` | `GET /v1/agents` | Wrapper `{agents: [AgentSummary]}`; covers `active`, `draft`, `paused` status values |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared fixture docs describe GET /v1/templates and GET /v1/agents as wrapper objects ({templates: [...]} / {agents: [...]}), but the fixtures, SDK parsers, and backend management endpoints use bare JSON arrays. The wire objects also use id; template_id / agent_id are SDK-level names. Please update these rows so other SDKs or backend tests do not implement the wrapper shape by mistake.

… fixture docs

Two review findings.

agents.create / create_from_template fail-closed
generate_runtime_key=True is a promise the response carries a one-time
runtime key. _parse_agent_create only validated the 'agent' block, so a
response omitting 'api_key' yielded runtime_full_key=None -- identical to
the generate_runtime_key=False path. A caller then doing
AgentClient(api_key=result.runtime_full_key) passes None, which
_BaseClient resolves via `api_key or os.environ[...]`, silently using
XAGENT_API_KEY -- a different agent's credential. Add _require_runtime_key
to both create paths: when a key was requested but the response carried
none, raise MalformedResponse instead of returning a keyless result.

shared/README.md fixture contract
The GET /v1/templates and GET /v1/agents rows described wrapper objects
({templates: [...]} / {agents: [...]}) but the fixtures, parsers, and
backend return bare JSON arrays keyed by `id`. Corrected those rows, the
agents_create row (nested {agent, api_key} shape), and the stale error
count so other-language clients implement the real wire contract.
The earlier wording cleanup covered src/ and unit tests but missed the
e2e suite and the public README. Remove the version anchors and roadmap
labels that describe how the codebase evolved rather than what it does:

- e2e: drop "(0.2.0)" / "Phase 2" / "no longer exposes" from module and
  test docstrings; rename test_e2e_full_flow_phase2 -> test_e2e_full_flow.
- e2e: default E2E_TEMPLATE_ID to the first listed template instead of a
  hardcoded id, and skip (not fail) when the backend lists no templates.
- README: drop "Phase 2 happy path", "Phase 3 roadmap", and
  "reserved; backend does not yet emit"; the migration guide and semver
  notes keep their version references because that is their purpose.
- README examples used invented template ids; switch to a real
  backend-defined id and note the set is backend-defined.
- Rename test_check_no_legacy_callsites.py -> test_forbidden_symbols.py
  and the leftover "from_legacy" test-data string -> "from_fallback".
@AlexLiu190625 AlexLiu190625 requested a review from rogercloud May 31, 2026 07:43

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the runtime-key fail-closed check.

Comment thread python/src/xagent_sdk/_agents.py Outdated
a *different* agent's credential. Raise instead of handing back a
keyless result that invites that fallback.
"""
if generate_runtime_key and result.runtime_full_key is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now fails closed for a missing runtime key, but it still lets an empty full_key through. AgentClient(api_key="") is also dangerous because _BaseClient resolves keys with api_key or os.environ.get(...), so an empty string falls back to XAGENT_API_KEY and can silently authenticate as a different agent. Please reject all empty runtime keys here, for example if generate_runtime_key and not result.runtime_full_key:, and cover the empty-string case for both create() and create_from_template().

_require_runtime_key tested `runtime_full_key is None`, but _BaseClient
resolves the key with `api_key or os.environ.get(...)` -- which falls
back to XAGENT_API_KEY for any falsy value, including an empty string.
A response carrying `full_key=""` therefore slipped past the guard and
let AgentClient silently authenticate as a different agent. Switch the
check to `not result.runtime_full_key` so it mirrors the same falsiness
the resolver uses. Cover the empty-string case for both create() and
create_from_template().
…all site

The runtime-key footgun kept reappearing because the empty/missing-key
invariant was being patched at each call site (agents.create paths)
while the actual sharp edge lived in _BaseClient: it resolved keys with
`api_key = api_key or os.environ.get(...)`, so any falsy explicit value
-- None or "" -- was silently swapped for XAGENT_API_KEY, authenticating
as a different agent.

Fix it at the boundary instead. _BaseClient now falls back to the
environment only when the argument was omitted (is None); an explicitly
passed empty string reaches the existing `if not api_key: raise` guard
and fails. Applied to both api_key and base_url, since both shared the
same lossy `or` resolution.

This splits responsibility cleanly:
- "" (and any non-None falsy) -> rejected at _BaseClient construction;
  no call site can route it to the environment.
- None from a create response (backend omitted the key block) -> still
  caught earlier by _require_runtime_key at create() time, because None
  is exactly the value _BaseClient treats as "use env", so the create
  guard must resolve the ambiguity with its knowledge that
  generate_runtime_key=True was requested. Reverted that guard from the
  `not`-based call-site patch back to an `is None` check scoped to its
  real job (missing key block).

The empty-string fail-closed tests moved from the agents call sites to
_BaseClient construction tests (empty api_key / base_url do not fall back
to env), where the invariant now lives.
@AlexLiu190625 AlexLiu190625 requested a review from rogercloud May 31, 2026 09:31

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after rechecking latest head f395f80. Local validation and GitHub CI are passing.

@AlexLiu190625 AlexLiu190625 merged commit 4902de3 into xorbitsai:main May 31, 2026
3 checks passed
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