From e8c021bc1094ef659abcd00a2cb1978556841778 Mon Sep 17 00:00:00 2001 From: Yao Date: Fri, 25 Sep 2026 17:17:07 +0800 Subject: [PATCH] feat(sdk): expose the error envelope's type and code on API errors (#470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ManagedAgentsApiError` exposed only `status` and a message built from `error.message`, so the runtime's error taxonomy was unreachable from the SDK: API error 400: agent must be a standard agent id The published envelope is structural, and the runtime builds it in one place (`src/api/routes/sessions.ts:766`): {"error":{"type":"invalid_request_error","code":"invalid_agent_ref","message":"..."}} A caller is meant to branch on that identity. Without it, distinguishing a wrong agent reference from a malformed body means substring-matching English prose that is not a stable interface, and any rewording of a message is a silent breaking change for the consumer that was forced to match on it. This closes the client half of the error taxonomy (#464) and does not change the canonical type (D11). * `error.type` and `error.code` carry the runtime's own values, and are `undefined` when the response carried no envelope or named no specific cause (`not_found` is a type, not a code under `invalid_request_error`). * The message is deliberately **unchanged** — `API error 400: agent must be a standard agent id` — so this is additive for anyone already matching on it. Measured: only one assertion in the suite matches an error message (`tests/integration/sdk.test.ts:251`, `/API error 404/`). * The error body is now read exactly once. The previous implementation called `res.json()` and then `res.text()` on the same `Response`, whose body is single-use, so the non-JSON fallback could never succeed: a non-JSON failure was reported as `statusText`, not as the body the runtime sent. The fallback was dead code that looked alive. * `requestText` — which backs artifact and file content plus metrics — goes through the same reader. It used the raw body as the message, so a JSON envelope was reported as raw JSON text with no fields readable. * A JSON body that is not an envelope keeps the previous `statusText` fallback, so only the envelope's fields are newly reachable; an unrelated JSON payload cannot leak into a log line. Verification: `npm run release:check` — see PR body for counts. The integration test drives a real listener over real SQLite, so the codes are produced by the routes: `invalid_agent_ref` and `agent_required` from `POST /v1/sessions`, `not_found` from `GET /v1/sessions/{id}` and from the `requestText` path, and the message pinned byte-for-byte so adding fields cannot alter the sentence existing callers log. The non-JSON and malformed-envelope branches are covered by a stubbed fetch, because no route in the suite answers a failure with a non-JSON body. Not covered: no Docker daemon, no Kubernetes cluster, and no model provider credentials exist on this host, so no provider boundary is exercised; `invalid_initial_events` was not reachable, because that route resolves a default environment before it validates `initial_events` and answers `Environment not found: env_default` with no code. Reverse probes, each differing from the fix in one respect: A. type/code dropped from the constructor -> see PR body B. body read twice, as before -> see PR body C. envelope required to be read from the raw text rather than parsed -> see PR body --- CHANGELOG.md | 2 + docs/usage.md | 24 +++ src/sdk/client.ts | 67 +++++++-- tests/integration/sdk-error-envelope.test.ts | 150 +++++++++++++++++++ tests/unit/sdk-client.test.ts | 79 +++++++++- 5 files changed, 310 insertions(+), 12 deletions(-) create mode 100644 tests/integration/sdk-error-envelope.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e265f0c..a225ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Added +- Exposes the published error envelope's `type` and `code` on `ManagedAgentsApiError`, closing the client half of the error taxonomy. The envelope is structural — `{"error":{"type":"invalid_request_error","code":"invalid_agent_ref","message":"..."}}` — and a caller is meant to branch on it, but the SDK read only `error.message`, so the runtime's entire taxonomy was unreachable and a caller wanting to tell a wrong agent reference from a malformed body had to substring-match English prose that is not a stable interface. `error.type` and `error.code` now carry the runtime's own values, which makes a future rewording of a message safe rather than a silent breaking change for a consumer that was forced to match on it — the code that the previous change had to print as prose (`model.api_key: TYPO is not set`) is now readable as a value. The message is deliberately **unchanged** (`API error 400: agent must be a standard agent id`), so this is additive for anyone already matching on it. The error body is also now read exactly once: the previous implementation called `res.json()` and then `res.text()` on the same `Response`, whose body is single-use, so the non-JSON fallback could never succeed — a non-JSON failure was reported as `statusText` rather than as the body the runtime actually sent, and the fallback was dead code that looked alive. `requestText`, which backs artifact and file content plus metrics, goes through the same reader, so a JSON envelope is no longer reported as raw JSON text with no fields readable. + - Registers the documented `managed-agents settings` group and moves the SDK's canonical runtime settings helpers onto the routes they claim to call. The group is documented as covered in `docs/api-matrix.md:86` — "Get, set model boundary, and validate canonical runtime settings" — and ticked in `docs/spec/tasks.md:144`, and its three commands are implemented in `src/cli/runtime-management-commands.ts`, the module the previous change registered only halfway: these are the commands it deliberately left unregistered rather than register broken (**#466**). Measured against a live listener, `settings get` threw `Cannot read properties of undefined (reading 'metadata')`, `settings validate` threw `result.checks is not iterable`, and `settings set-model` failed with `API error 404: No route matches this request`. The first two read a document taken from a type no route produces — the runtime's internal registry report, with `model_provider`, `loop_engine.implemented`, `storage.metadata.type` and `validation.checks` — so every field a reader could reach for was `undefined` at runtime while type-checking; the third sent `PATCH /v1/x/settings`, where the route mounts `PUT`. `settings.patch()` is therefore a read-merge-write: it reads the current `revision` and `saved_config`, merges the partial document over it, validates the result, and writes it with the revision it read, so a masked secret survives the round trip and a concurrent writer is refused with `409` rather than silently overwritten — there is no automatic retry, because the SDK cannot know whether re-applying the patch is still meaningful. A document the runtime would reject throws `RuntimeSettingsValidationError` before the write, naming each failing field, including the environment variable behind a `${NAME}` reference that does not resolve in the **runtime's** environment; that detail is precisely what the shared error path drops (**#464**), and without it `settings set-model --api-key-env TYPO` would report only `Settings configuration is invalid`. The SDK's `RuntimeSettingsSummary` and `RuntimeSettingsPatch` now describe the routes' own fields, secrets are documented as surviving the round trip because reads mask a stored key as `********` and the store treats that sentinel as "keep", and an integration test drives a real HTTP listener through a recording `fetch` that delegates to it rather than replacing it, so the verb asserted is the verb served. - Registers the documented `managed-agents workspace` group. All five commands — `create`, `open`, `list`, `resolve`, `remove` — are implemented in `src/cli/workspace-commands.ts`, documented as covered in `docs/api-matrix.md:88`, and ticked off in `docs/spec/tasks.md:192`, but the module was imported by nothing, so every one answered `error: unknown command 'workspace'`. This is the fourth and last instance of that pattern (#459/#463 worker, #460/#465 session, #461/#467 environments). It is also the one group that is not an HTTP client: `src/core/workspace/registry.ts` reads and writes `$MANAGED_AGENTS_HOME/workspaces.json`, so these commands take no `--port` or `--api-key`, and the whole workspace-registry subsystem becomes reachable for the first time — the orphaned CLI module was its **only** consumer, so before this it was dead code with a ticked task-list entry. Tests point the registry at a temporary directory through `MANAGED_AGENTS_HOME`, which is what makes them runnable without writing to the developer's real `~/.managed-agents`; that variable was documented only in `CONTRIBUTING.md` and is now in `docs/usage.md` as well, since the CLI is the surface that needs it. Three behaviors are pinned that the printed output alone would not catch. `create` scaffolds `/agents`, `/skills`, and `/.managed-agents/config.yaml`, asserted **on disk**, while `open` must create nothing at all — asserted by the absence of both scaffold directories, so the two are not interchangeable. `remove` drops the registry entry and **leaves every file behind**, which is asserted because the same command turning into a recursive delete would destroy a user's agents and sessions without a prompt. And `resolve` is not a read: it bumps `last_opened_at` and therefore reorders the listing, so a caller resolving in a loop silently rewrites the registry. `resolve` and `remove` set `process.exitCode = 1` on a miss, which the tests capture and restore rather than leaving set. Documentation updated in `docs/usage.md`: the registry location and `MANAGED_AGENTS_HOME` under Workspace Layout, the five commands in the CLI list, and a paragraph distinguishing `create` from `open`. diff --git a/docs/usage.md b/docs/usage.md index 0ab053f..ed2c59a 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -414,6 +414,30 @@ for await (const event of client.sessions.chat(session.id, 'Hello')) { } ``` +A refused request throws `ManagedAgentsApiError`, which carries the published +error envelope's identity as well as its prose: `status`, `type` (for example +`invalid_request_error`, `not_found`, or `conflict`), and `code` when the +runtime names a specific cause (`invalid_agent_ref`, `budget_reached`, +`unsupported_model_field`, and others). Branch on those instead of on the +message, which is prose and may be reworded: + +```typescript +import { ManagedAgentsApiError } from 'managed-agents/sdk'; + +try { + await client.sessions.create({ agent: 'assistant' }); +} catch (error) { + if (error instanceof ManagedAgentsApiError && error.code === 'invalid_agent_ref') { + // `agent` takes an agent id, not a name. + } + throw error; +} +``` + +`type` and `code` are `undefined` when the response carried no envelope or the +runtime named no specific cause. The message is unchanged, so existing code that +matches on it keeps working. + ## CLI Commands ```bash diff --git a/src/sdk/client.ts b/src/sdk/client.ts index 2f88d60..a8e97ca 100644 --- a/src/sdk/client.ts +++ b/src/sdk/client.ts @@ -414,14 +414,8 @@ export class ManagedAgentsClient { }); if (!res.ok) { - let detail = ''; - try { - const j = (await res.json()) as { error?: { message?: string } }; - detail = j.error?.message ?? ''; - } catch { - detail = await res.text().catch(() => ''); - } - throw new ManagedAgentsApiError(res.status, detail || res.statusText); + const envelope = await readErrorEnvelope(res); + throw new ManagedAgentsApiError(res.status, envelope.message || res.statusText, envelope); } if (res.status === 204) return undefined as T; @@ -435,7 +429,8 @@ export class ManagedAgentsClient { const res = await this.fetchImpl(`${this.baseUrl}${path}`, { method, headers }); if (!res.ok) { - throw new ManagedAgentsApiError(res.status, await res.text().catch(() => res.statusText)); + const envelope = await readErrorEnvelope(res); + throw new ManagedAgentsApiError(res.status, envelope.message || res.statusText, envelope); } return res.text(); } @@ -917,13 +912,67 @@ class EnvironmentsResource { // Errors + SSE parsing // ============================================================ +/** + * The published error envelope: `{"error":{"type":..., "code":..., "message":...}}`. + * + * `type` is the canonical error class (D11: `invalid_request_error`, with `not_found` and + * `conflict` deliberately kept as their own), and `code` names a specific cause within it. + * A caller branches on these; the message is prose and is not a stable interface. + */ +export interface ApiErrorEnvelope { + type?: string; + code?: string; +} + export class ManagedAgentsApiError extends Error { + /** + * The envelope's `type`, e.g. `invalid_request_error`. + * + * `undefined` when the response carried no envelope — a non-JSON body, or a transport that + * failed before the runtime answered. + */ + readonly type?: string; + /** The envelope's `code`, e.g. `invalid_agent_ref`, when the cause is specific enough to name. */ + readonly code?: string; + constructor( public readonly status: number, message: string, + envelope: ApiErrorEnvelope = {}, ) { super(`API error ${status}: ${message}`); this.name = 'ManagedAgentsApiError'; + this.type = envelope.type; + this.code = envelope.code; + } +} + +/** + * Read a failed response's published envelope, consuming the body exactly once. + * + * The previous version called `res.json()` and then `res.text()` on the same response. A + * `Response` body is single-use, so the fallback could never succeed — and `error.type` and + * `error.code` were never read at all, which is what made the runtime's whole error taxonomy + * unreachable from the SDK. + * + * A body that is not JSON is returned as the message, which is what the dead fallback was for; + * a JSON body that is not an envelope keeps the previous `statusText` fallback, so only the + * envelope's fields are newly reachable. + */ +async function readErrorEnvelope(res: Response): Promise<{ message: string } & ApiErrorEnvelope> { + const raw = await res.text().catch(() => ''); + if (!raw) return { message: '' }; + try { + const parsed = JSON.parse(raw) as { error?: { type?: unknown; code?: unknown; message?: unknown } } | null; + const error = parsed?.error; + if (!error || typeof error !== 'object') return { message: '' }; + return { + message: typeof error.message === 'string' ? error.message : '', + ...(typeof error.type === 'string' ? { type: error.type } : {}), + ...(typeof error.code === 'string' ? { code: error.code } : {}), + }; + } catch { + return { message: raw }; } } diff --git a/tests/integration/sdk-error-envelope.test.ts b/tests/integration/sdk-error-envelope.test.ts new file mode 100644 index 0000000..da8c57f --- /dev/null +++ b/tests/integration/sdk-error-envelope.test.ts @@ -0,0 +1,150 @@ +/** + * Integration test: the published error envelope's `type` and `code` reach the SDK caller. + * + * `ManagedAgentsApiError` exposed only `status` and a message built from `error.message`, so + * the runtime's error taxonomy was unreachable from the SDK: a caller wanting to tell a wrong + * agent reference from a malformed body had to substring-match English prose that is not a + * stable interface. The envelope is structural — + * `{"error":{"type":"invalid_request_error","code":"invalid_agent_ref","message":"..."}}` — + * and `src/api/routes/sessions.ts:766` builds it in one place. + * + * Every assertion here is against a **real listener over real SQLite**: the codes are produced + * by the routes themselves, so a test that agreed with a wrong SDK would fail. The message is + * also pinned byte-for-byte, because adding fields must not change the sentence existing + * callers already log. + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { join } from 'node:path'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { serve } from '@hono/node-server'; +import { Database } from '@/core/db/database.js'; +import { SessionManager } from '@/core/session/session-manager.js'; +import { createServer } from '@/api/server.js'; +import { ManagedAgentsApiError, ManagedAgentsClient } from '@/sdk/client.js'; + +type RealServer = { close: (cb?: () => void) => void; closeAllConnections?: () => void }; + +describe('SDK error envelope', () => { + let db: Database | undefined; + let tmpDir: string | undefined; + let listening: RealServer | undefined; + + afterEach(async () => { + if (listening) { + const server = listening; + listening = undefined; + server.closeAllConnections?.(); + await new Promise((resolve) => server.close(() => resolve())); + } + db?.close(); + db = undefined; + if (tmpDir) rmSync(tmpDir, { recursive: true, force: true }); + tmpDir = undefined; + }); + + async function startRuntime() { + const dir = mkdtempSync(join(tmpdir(), 'ma-sdk-error-')); + tmpDir = dir; + const dataDir = join(dir, '.managed-agents'); + mkdirSync(dataDir, { recursive: true }); + db = new Database(join(dir, 'test.db')); + db.runMigrations(); + + const app = createServer({ + db, + sessionManager: new SessionManager(db), + agents: [], + reloadAgents: () => ({ agents: [], errors: [] }), + workspace: { + root: dir, + dataDir, + agentsDir: join(dir, 'agents'), + skillsDir: join(dir, 'skills'), + configPath: join(dir, 'managed-agents.config.yaml'), + target: 'local', + }, + }); + + const port = await new Promise((resolve) => { + const server = serve({ fetch: app.fetch, port: 0, hostname: '127.0.0.1' }, (info) => { + listening = server as unknown as RealServer; + resolve(info.port); + }); + }); + + return { client: new ManagedAgentsClient({ baseUrl: `http://127.0.0.1:${port}` }), app }; + } + + /** Run a call whose refusal is the point, and return the error it threw. */ + async function captureError(run: () => Promise): Promise { + const failure = await run().then(() => undefined, (error: unknown) => error); + expect(failure).toBeInstanceOf(ManagedAgentsApiError); + return failure as ManagedAgentsApiError; + } + + it('carries the canonical type and the specific code of a real 400', async () => { + const { client } = await startRuntime(); + + const error = await captureError(() => client.sessions.create({ agent: 'not-an-agent-id' })); + + expect(error.status).toBe(400); + // The whole point: a caller can branch on this instead of matching prose. + expect(error.type).toBe('invalid_request_error'); + expect(error.code).toBe('invalid_agent_ref'); + // `code`/`type` are additive: the sentence existing callers log is unchanged, so this is + // not a breaking change for anyone already matching on it. + expect(error.message).toBe('API error 400: agent must be a standard agent id'); + }); + + it('carries a different code from the same route, so the field is read not guessed', async () => { + const { client } = await startRuntime(); + + // A second code from a second failure on the same path: if the SDK were reporting a + // constant, or reading the wrong field, one of these two would disagree. + const error = await captureError(() => client.sessions.create({} as never)); + + expect(error.status).toBe(400); + expect(error.type).toBe('invalid_request_error'); + expect(error.code).toBe('agent_required'); + expect(error.message).toBe('API error 400: agent field is required'); + }); + + it('reports a type with no code when the refusal names no specific cause', async () => { + const { client } = await startRuntime(); + + const error = await captureError(() => client.sessions.get('sess_does_not_exist')); + + expect(error.status).toBe(404); + // `not_found` is deliberately its own type, not a code under `invalid_request_error` + // (D11) — and this refusal carries no code. + expect(error.type).toBe('not_found'); + expect(error.code).toBeUndefined(); + expect(error.message).toBe('API error 404: Session not found'); + }); + + it('carries the envelope on the text-response path too', async () => { + const { client } = await startRuntime(); + + // `requestText` backs artifact and file content plus metrics. It built its error from the + // raw body, so a JSON envelope was reported as raw JSON with no fields readable. + const error = await captureError(() => client.sessions.artifactText('sess_missing', 'art_missing')); + + expect(error.status).toBe(404); + expect(error.type).toBe('not_found'); + // The prose, not the `{"error":{...}}` body that the raw-text path used to report. + expect(error.message).not.toContain('{'); + expect(error.message).toContain('not found'); + }); + + it('is an Error, so existing catch blocks and rethrows keep working', async () => { + const { client } = await startRuntime(); + + const error = await captureError(() => client.sessions.get('sess_does_not_exist')); + + expect(error).toBeInstanceOf(Error); + expect(error.name).toBe('ManagedAgentsApiError'); + expect(typeof error.stack).toBe('string'); + }); +}); diff --git a/tests/unit/sdk-client.test.ts b/tests/unit/sdk-client.test.ts index fc12139..4e8d1f9 100644 --- a/tests/unit/sdk-client.test.ts +++ b/tests/unit/sdk-client.test.ts @@ -1,5 +1,77 @@ import { describe, expect, it, vi } from 'vitest'; -import { ManagedAgentsClient } from '@/sdk/client.js'; +import { ManagedAgentsApiError, ManagedAgentsClient } from '@/sdk/client.js'; + +describe('ManagedAgentsClient error envelope', () => { + const clientReturning = (response: Response) => + new ManagedAgentsClient({ + baseUrl: 'http://localhost:3000', + fetch: (async () => response) as unknown as typeof fetch, + }); + + it('reports a non-JSON error body as the message', async () => { + // A stubbed fetch, because no route in the suite answers a failure with a non-JSON body: + // this covers the fallback branch, which is also the branch that was dead. The previous + // implementation read `res.json()` and then `res.text()` on the same response, so the + // single-use body was already consumed and the fallback always produced `statusText`. + const client = clientReturning(new Response('upstream is unavailable', { status: 503 })); + + const error = await client.sessions.get('sess_1').then(() => undefined, (e: unknown) => e) as ManagedAgentsApiError; + + expect(error).toBeInstanceOf(ManagedAgentsApiError); + expect(error.status).toBe(503); + // The body that was actually sent, not `Service Unavailable`. + expect(error.message).toBe('API error 503: upstream is unavailable'); + expect(error.type).toBeUndefined(); + expect(error.code).toBeUndefined(); + }); + + it('falls back to the status text for an empty error body', async () => { + const client = clientReturning(new Response('', { status: 502, statusText: 'Bad Gateway' })); + + const error = await client.sessions.get('sess_1').then(() => undefined, (e: unknown) => e) as ManagedAgentsApiError; + + expect(error.message).toBe('API error 502: Bad Gateway'); + }); + + it('falls back to the status text for JSON that is not an envelope', async () => { + // A JSON body without an `error` object is not the published envelope, so it must not be + // reported as the message — otherwise an unrelated JSON payload would leak into logs. + const client = clientReturning(jsonResponse({ valid: false }, 500, 'Internal Server Error')); + + const error = await client.sessions.get('sess_1').then(() => undefined, (e: unknown) => e) as ManagedAgentsApiError; + + expect(error.message).toBe('API error 500: Internal Server Error'); + }); + + it('reads type and code from a well-formed envelope', async () => { + const response = jsonResponse( + { error: { type: 'invalid_request_error', code: 'invalid_agent_ref', message: 'agent must be a standard agent id' } }, + 400, + ); + const client = clientReturning(response); + + const error = await client.sessions.get('sess_1').then(() => undefined, (e: unknown) => e) as ManagedAgentsApiError; + + expect(error.type).toBe('invalid_request_error'); + expect(error.code).toBe('invalid_agent_ref'); + expect(error.message).toBe('API error 400: agent must be a standard agent id'); + }); + + it('ignores non-string envelope fields instead of reporting them', async () => { + // A malformed envelope must not put `[object Object]` or a number into a typed field. + const response = jsonResponse( + { error: { type: 7, code: { nested: true }, message: 'bad request' } }, + 400, + ); + const client = clientReturning(response); + + const error = await client.sessions.get('sess_1').then(() => undefined, (e: unknown) => e) as ManagedAgentsApiError; + + expect(error.type).toBeUndefined(); + expect(error.code).toBeUndefined(); + expect(error.message).toBe('API error 400: bad request'); + }); +}); describe('ManagedAgentsClient runtime management resources', () => { it('sends tool confirmation and custom tool result events', async () => { @@ -139,9 +211,10 @@ describe('ManagedAgentsClient runtime management resources', () => { }); }); -function jsonResponse(value: unknown): Response { +function jsonResponse(value: unknown, status = 200, statusText?: string): Response { return new Response(JSON.stringify(value), { - status: 200, + status, + ...(statusText === undefined ? {} : { statusText }), headers: { 'Content-Type': 'application/json' }, }); }