diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index 6c147e6d..06e558dc 100644 --- a/docs/docs/api/appkit/Class.Plugin.md +++ b/docs/docs/api/appkit/Class.Plugin.md @@ -313,14 +313,16 @@ BasePlugin.clientConfig protected execute( fn: (signal?: AbortSignal) => Promise, options: PluginExecutionSettings, -userKey?: string): Promise; +userKey?: string): Promise>; ``` Execute a function with the plugin's interceptor chain. -All errors are caught and `undefined` is returned (production-safe). -Route handlers should check for `undefined` and respond with an -appropriate error status. +Returns an [ExecutionResult](TypeAlias.ExecutionResult.md) discriminated union: +- `{ ok: true, data: T }` on success +- `{ ok: false, status: number, message: string }` on failure + +Errors are never thrown — the method is production-safe. #### Type Parameters @@ -338,7 +340,7 @@ appropriate error status. #### Returns -`Promise`\<`T` \| `undefined`\> +`Promise`\<[`ExecutionResult`](TypeAlias.ExecutionResult.md)\<`T`\>\> *** diff --git a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md new file mode 100644 index 00000000..1af9686e --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md @@ -0,0 +1,33 @@ +# Type Alias: ExecutionResult\ + +```ts +type ExecutionResult = + | { + data: T; + ok: true; +} + | { + message: string; + ok: false; + status: number; +}; +``` + +Discriminated union for plugin execution results. + +Replaces the previous `T | undefined` return type on `execute()`. + +On failure, the HTTP status code is preserved from: +- `AppKitError` subclasses (via `statusCode`) +- Any `Error` with a numeric `statusCode` property (e.g. `ApiError`) +- All other errors default to status 500 + +In production, error messages from non-AppKitError sources are handled as: +- 4xx errors: original message is preserved (client-facing by design) +- 5xx errors: replaced with "Server error" to prevent information leakage + +## Type Parameters + +| Type Parameter | +| ------ | +| `T` | diff --git a/docs/docs/api/appkit/index.md b/docs/docs/api/appkit/index.md index f4685e04..05c69e57 100644 --- a/docs/docs/api/appkit/index.md +++ b/docs/docs/api/appkit/index.md @@ -54,6 +54,7 @@ plugin architecture, and React integration. | Type Alias | Description | | ------ | ------ | | [ConfigSchema](TypeAlias.ConfigSchema.md) | Configuration schema definition for plugin config. Re-exported from the standard JSON Schema Draft 7 types. | +| [ExecutionResult](TypeAlias.ExecutionResult.md) | Discriminated union for plugin execution results. | | [IAppRouter](TypeAlias.IAppRouter.md) | Express router type for plugin route registration | | [PluginData](TypeAlias.PluginData.md) | Tuple of plugin class, config, and name. Created by `toPlugin()` and passed to `createApp()`. | | [ResourcePermission](TypeAlias.ResourcePermission.md) | Union of all possible permission levels across all resource types. | diff --git a/docs/docs/api/appkit/typedoc-sidebar.ts b/docs/docs/api/appkit/typedoc-sidebar.ts index 91815e3d..0bc09366 100644 --- a/docs/docs/api/appkit/typedoc-sidebar.ts +++ b/docs/docs/api/appkit/typedoc-sidebar.ts @@ -183,6 +183,11 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/TypeAlias.ConfigSchema", label: "ConfigSchema" }, + { + type: "doc", + id: "api/appkit/TypeAlias.ExecutionResult", + label: "ExecutionResult" + }, { type: "doc", id: "api/appkit/TypeAlias.IAppRouter", diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 662a9178..836686af 100644 --- a/packages/appkit/src/index.ts +++ b/packages/appkit/src/index.ts @@ -47,7 +47,12 @@ export { ValidationError, } from "./errors"; // Plugin authoring -export { Plugin, type ToPlugin, toPlugin } from "./plugin"; +export { + type ExecutionResult, + Plugin, + type ToPlugin, + toPlugin, +} from "./plugin"; export { analytics, files, genie, lakebase, server, serving } from "./plugins"; export type { EndpointConfig, diff --git a/packages/appkit/src/plugin/execution-result.ts b/packages/appkit/src/plugin/execution-result.ts new file mode 100644 index 00000000..964e8983 --- /dev/null +++ b/packages/appkit/src/plugin/execution-result.ts @@ -0,0 +1,17 @@ +/** + * Discriminated union for plugin execution results. + * + * Replaces the previous `T | undefined` return type on `execute()`. + * + * On failure, the HTTP status code is preserved from: + * - `AppKitError` subclasses (via `statusCode`) + * - Any `Error` with a numeric `statusCode` property (e.g. `ApiError`) + * - All other errors default to status 500 + * + * In production, error messages from non-AppKitError sources are handled as: + * - 4xx errors: original message is preserved (client-facing by design) + * - 5xx errors: replaced with "Server error" to prevent information leakage + */ +export type ExecutionResult = + | { ok: true; data: T } + | { ok: false; status: number; message: string }; diff --git a/packages/appkit/src/plugin/index.ts b/packages/appkit/src/plugin/index.ts index 39e0ee65..93765219 100644 --- a/packages/appkit/src/plugin/index.ts +++ b/packages/appkit/src/plugin/index.ts @@ -1,3 +1,4 @@ export type { ToPlugin } from "shared"; +export type { ExecutionResult } from "./execution-result"; export { Plugin } from "./plugin"; export { toPlugin } from "./to-plugin"; diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 3cd1c834..5173cb61 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -19,7 +19,7 @@ import { ServiceContext, type UserContext, } from "../context"; -import { AuthenticationError } from "../errors"; +import { AppKitError, AuthenticationError } from "../errors"; import { createLogger } from "../logging/logger"; import { StreamManager } from "../stream"; import { @@ -29,6 +29,7 @@ import { } from "../telemetry"; import { deepMerge } from "../utils"; import { DevFileReader } from "./dev-reader"; +import type { ExecutionResult } from "./execution-result"; import { CacheInterceptor } from "./interceptors/cache"; import { RetryInterceptor } from "./interceptors/retry"; import { TelemetryInterceptor } from "./interceptors/telemetry"; @@ -40,6 +41,20 @@ import type { const logger = createLogger("plugin"); +/** + * Narrow an unknown thrown value to an Error that carries a numeric + * `statusCode` property (e.g. `ApiError` from `@databricks/sdk-experimental`). + */ +function hasHttpStatusCode( + error: unknown, +): error is Error & { statusCode: number } { + return ( + error instanceof Error && + "statusCode" in error && + typeof (error as Record).statusCode === "number" + ); +} + /** * Methods that should not be proxied by asUser(). * These are lifecycle/internal methods that don't make sense @@ -435,15 +450,17 @@ export abstract class Plugin< /** * Execute a function with the plugin's interceptor chain. * - * All errors are caught and `undefined` is returned (production-safe). - * Route handlers should check for `undefined` and respond with an - * appropriate error status. + * Returns an {@link ExecutionResult} discriminated union: + * - `{ ok: true, data: T }` on success + * - `{ ok: false, status: number, message: string }` on failure + * + * Errors are never thrown — the method is production-safe. */ protected async execute( fn: (signal?: AbortSignal) => Promise, options: PluginExecutionSettings, userKey?: string, - ): Promise { + ): Promise> { const executeConfig = this._buildExecutionConfig(options); const interceptors = this._buildInterceptors(executeConfig); @@ -457,11 +474,40 @@ export abstract class Plugin< }; try { - return await this._executeWithInterceptors(fn, interceptors, context); + const data = await this._executeWithInterceptors( + fn, + interceptors, + context, + ); + return { ok: true, data }; } catch (error) { - // production-safe: swallow all errors, don't crash the app logger.error("Plugin execution failed", { error, plugin: this.name }); - return undefined; + + if (error instanceof AppKitError) { + return { + ok: false, + status: error.statusCode, + message: error.message, + }; + } + + if (hasHttpStatusCode(error)) { + const isDev = process.env.NODE_ENV !== "production"; + const isClientError = error.statusCode >= 400 && error.statusCode < 500; + return { + ok: false, + status: error.statusCode, + message: isDev || isClientError ? error.message : "Server error", + }; + } + + const isDev = process.env.NODE_ENV !== "production"; + return { + ok: false, + status: 500, + message: + isDev && error instanceof Error ? error.message : "Server error", + }; } } diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index cc64e0a5..440579d7 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -9,6 +9,13 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AppManager } from "../../app"; import { CacheManager } from "../../cache"; import { ServiceContext } from "../../context/service-context"; +import { + AuthenticationError, + ConnectionError, + ExecutionError, + TunnelError, + ValidationError, +} from "../../errors"; import { StreamManager } from "../../stream"; import type { ITelemetry, TelemetryProvider } from "../../telemetry"; import { TelemetryManager } from "../../telemetry"; @@ -317,7 +324,7 @@ describe("Plugin", () => { }); describe("execute", () => { - test("should execute function with interceptors", async () => { + test("should return ok result on success", async () => { const plugin = new TestPlugin(config); const mockFn = vi.fn().mockResolvedValue("result"); @@ -328,54 +335,256 @@ describe("Plugin", () => { const result = await (plugin as any).execute(mockFn, options, false); - expect(result).toBe("result"); + expect(result).toEqual({ ok: true, data: "result" }); expect(mockFn).toHaveBeenCalledTimes(1); }); - test("should return undefined on non-API error (production-safe)", async () => { - const plugin = new TestPlugin(config); - const mockFn = vi.fn().mockRejectedValue(new Error("Test error")); - - const options = { - default: {}, - }; - - const result = await (plugin as any).execute(mockFn, options, false); + test("should return error result with status 500 for non-AppKitError in production", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new TestPlugin(config); + const mockFn = vi.fn().mockRejectedValue(new Error("secret details")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + + expect(result).toEqual({ + ok: false, + status: 500, + message: "Server error", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); - expect(result).toBeUndefined(); + test("should return original message for non-AppKitError in development", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "development"; + try { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(new Error("detailed debug info")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + + expect(result).toEqual({ + ok: false, + status: 500, + message: "detailed debug info", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } }); - test("should swallow ApiError and return undefined", async () => { + test("should preserve 404 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); const apiError = new MockApiError("Not found", 404); const mockFn = vi.fn().mockRejectedValue(apiError); - const options = { default: {} }; - - const result = await (plugin as any).execute(mockFn, options, false); - expect(result).toBeUndefined(); + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 404, + message: "Not found", + }); }); - test("should swallow ApiError 401 and return undefined", async () => { + test("should preserve 401 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); const apiError = new MockApiError("Unauthorized", 401); const mockFn = vi.fn().mockRejectedValue(apiError); - const options = { default: {} }; - - const result = await (plugin as any).execute(mockFn, options, false); - expect(result).toBeUndefined(); + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 401, + message: "Unauthorized", + }); }); - test("should swallow ApiError 403 and return undefined", async () => { + test("should preserve 403 statusCode from ApiError (non-AppKitError)", async () => { const plugin = new TestPlugin(config); const apiError = new MockApiError("Forbidden", 403); const mockFn = vi.fn().mockRejectedValue(apiError); - const options = { default: {} }; + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 403, + message: "Forbidden", + }); + }); - const result = await (plugin as any).execute(mockFn, options, false); - expect(result).toBeUndefined(); + test("should preserve 502 statusCode from non-AppKitError", async () => { + const plugin = new TestPlugin(config); + const apiError = new MockApiError("Bad gateway", 502); + const mockFn = vi.fn().mockRejectedValue(apiError); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 502, + message: "Bad gateway", + }); + }); + + test("should redact message for 5xx statusCode errors in production", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new TestPlugin(config); + const apiError = new MockApiError("Internal upstream detail", 502); + const mockFn = vi.fn().mockRejectedValue(apiError); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 502, + message: "Server error", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("should preserve message for 4xx statusCode errors in production", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new TestPlugin(config); + const apiError = new MockApiError("Forbidden", 403); + const mockFn = vi.fn().mockRejectedValue(apiError); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 403, + message: "Forbidden", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("should map AuthenticationError to status 401", async () => { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(AuthenticationError.missingToken()); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 401, + message: expect.any(String), + }); + }); + + test("should map ValidationError to status 400", async () => { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(new ValidationError("bad input")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ ok: false, status: 400, message: "bad input" }); + }); + + test("should map ConnectionError to status 503", async () => { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(ConnectionError.apiFailure("test-service")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 503, + message: expect.any(String), + }); + }); + + test("should map TunnelError to status 502", async () => { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(new TunnelError("gateway failed")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 502, + message: "gateway failed", + }); + }); + + test("should map ExecutionError to status 500", async () => { + const plugin = new TestPlugin(config); + const mockFn = vi + .fn() + .mockRejectedValue(ExecutionError.statementFailed("query broke")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 500, + message: expect.any(String), + }); }); }); @@ -704,7 +913,7 @@ describe("Plugin", () => { user: { retry: { attempts: 3 } }, }); - expect(result).toBe("integration-result"); + expect(result).toEqual({ ok: true, data: "integration-result" }); }); }); }); diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 98a5da72..9344af85 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -1,3 +1,4 @@ +import { STATUS_CODES } from "node:http"; import { Readable } from "node:stream"; import { ApiError } from "@databricks/sdk-experimental"; import type express from "express"; @@ -424,6 +425,13 @@ export class FilesPlugin extends Plugin { res.status(500).json({ error: fallbackMessage, plugin: this.name }); } + private _sendStatusError(res: express.Response, status: number): void { + res.status(status).json({ + error: STATUS_CODES[status] ?? "Unknown Error", + plugin: this.name, + }); + } + private async _handleList( req: express.Request, res: express.Response, @@ -445,11 +453,11 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "List failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "List failed"); } @@ -481,11 +489,11 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "Read failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.type("text/plain").send(result); + res.type("text/plain").send(result.data); } catch (error) { this._handleApiError(res, error, "Read failed"); } @@ -545,8 +553,8 @@ export class FilesPlugin extends Plugin { return connector.download(getWorkspaceClient(), path); }, settings); - if (response === undefined) { - res.status(500).json({ error: `${label} failed`, plugin: this.name }); + if (!response.ok) { + this._sendStatusError(res, response.status); return; } @@ -575,16 +583,14 @@ export class FilesPlugin extends Plugin { ); } - if (response.contents) { + if (response.data.contents) { const nodeStream = Readable.fromWeb( - response.contents as import("node:stream/web").ReadableStream, + response.data.contents as import("node:stream/web").ReadableStream, ); nodeStream.on("error", (err) => { logger.error("Stream error during %s: %O", opts.mode, err); if (!res.headersSent) { - res - .status(500) - .json({ error: `${label} failed`, plugin: this.name }); + this._sendStatusError(res, 500); } else { res.destroy(); } @@ -624,13 +630,11 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res - .status(500) - .json({ error: "Exists check failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json({ exists: result }); + res.json({ exists: result.data }); } catch (error) { this._handleApiError(res, error, "Exists check failed"); } @@ -662,13 +666,11 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res - .status(500) - .json({ error: "Metadata fetch failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Metadata fetch failed"); } @@ -700,11 +702,11 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "Preview failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Preview failed"); } @@ -791,7 +793,7 @@ export class FilesPlugin extends Plugin { connector, ); - if (result === undefined) { + if (!result.ok) { logger.error( req, "Upload failed: volume=%s path=%s, size=%d bytes", @@ -799,12 +801,12 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - res.status(500).json({ error: "Upload failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } logger.debug(req, "Upload complete: volume=%s path=%s", volumeKey, path); - res.json(result); + res.json(result.data); } catch (error) { if ( error instanceof Error && @@ -851,14 +853,12 @@ export class FilesPlugin extends Plugin { connector, ); - if (result === undefined) { - res - .status(500) - .json({ error: "Create directory failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Create directory failed"); } @@ -898,12 +898,12 @@ export class FilesPlugin extends Plugin { connector, ); - if (result === undefined) { - res.status(500).json({ error: "Delete failed", plugin: this.name }); + if (!result.ok) { + this._sendStatusError(res, result.status); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Delete failed"); } diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index e0117cdc..55989148 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -567,7 +567,7 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(500); const data = (await response.json()) as { error: string; plugin: string }; - expect(data.error).toBe("Metadata fetch failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); @@ -583,11 +583,11 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(500); const data = (await response.json()) as { error: string; plugin: string }; - expect(data.error).toBe("List failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); - test("ApiError 404 is swallowed and returns 500", async () => { + test("ApiError 404 preserves upstream status code", async () => { mockFilesApi.getMetadata.mockRejectedValue( new MockApiError("Not found", 404), ); @@ -597,16 +597,16 @@ describe("Files Plugin Integration", () => { { headers: MOCK_AUTH_HEADERS }, ); - expect(response.status).toBe(500); + expect(response.status).toBe(404); const data = (await response.json()) as { error: string; plugin: string; }; - expect(data.error).toBe("Metadata fetch failed"); + expect(data.error).toBe("Not Found"); expect(data.plugin).toBe("files"); }); - test("ApiError 409 is swallowed and returns 500", async () => { + test("ApiError 409 preserves upstream status code", async () => { mockFilesApi.createDirectory.mockRejectedValue( new MockApiError("Conflict", 409), ); @@ -617,12 +617,12 @@ describe("Files Plugin Integration", () => { body: JSON.stringify({ path: "/Volumes/catalog/schema/vol/existing" }), }); - expect(response.status).toBe(500); + expect(response.status).toBe(409); const data = (await response.json()) as { error: string; plugin: string; }; - expect(data.error).toBe("Create directory failed"); + expect(data.error).toBe("Conflict"); expect(data.plugin).toBe("files"); }); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 27b55cd5..99e08b8c 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -674,7 +674,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ - error: "List failed", + error: "Internal Server Error", plugin: "files", }), ); @@ -698,7 +698,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Read failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -722,7 +722,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Exists check failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -746,7 +746,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Metadata fetch failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -768,7 +768,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Download failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -791,7 +791,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Create directory failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -835,7 +835,7 @@ describe("FilesPlugin", () => { await handlerPromise; const errorBody = res.json.mock.calls[0][0]; - expect(errorBody.error).toBe("List failed"); + expect(errorBody.error).toBe("Internal Server Error"); expect(errorBody.error).not.toContain("secret"); expect(errorBody.error).not.toContain("internal"); }); @@ -876,7 +876,7 @@ describe("FilesPlugin", () => { expect(signalWasAborted).toBe(true); expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "List failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); diff --git a/packages/appkit/src/plugins/serving/serving.ts b/packages/appkit/src/plugins/serving/serving.ts index f64f6c95..143e8bc5 100644 --- a/packages/appkit/src/plugins/serving/serving.ts +++ b/packages/appkit/src/plugins/serving/serving.ts @@ -6,7 +6,7 @@ import type { IAppRouter } from "shared"; import * as servingConnector from "../../connectors/serving/client"; import { getWorkspaceClient } from "../../context"; import { createLogger } from "../../logging"; -import { Plugin, toPlugin } from "../../plugin"; +import { type ExecutionResult, Plugin, toPlugin } from "../../plugin"; import type { PluginManifest, ResourceRequirement } from "../../registry"; import { ResourceType } from "../../registry"; import { servingInvokeDefaults } from "./defaults"; @@ -182,11 +182,11 @@ export class ServingPlugin extends Plugin { try { const result = await this.invoke(alias, rawBody); - if (result === undefined) { - res.status(502).json({ error: "Invocation returned no result" }); + if (!result.ok) { + res.status(result.status).json({ error: result.message }); return; } - res.json(result); + res.json(result.data); } catch (err) { const message = err instanceof Error ? err.message : "Invocation failed"; if (err instanceof EndpointNotFoundError) { @@ -264,7 +264,10 @@ export class ServingPlugin extends Plugin { } } - async invoke(alias: string, body: Record): Promise { + async invoke( + alias: string, + body: Record, + ): Promise> { const { endpoint, filteredBody } = this.resolveAndFilter(alias, body); const workspaceClient = getWorkspaceClient(); const timeout = this.config.timeout ?? 120_000; diff --git a/packages/appkit/src/plugins/serving/tests/serving.test.ts b/packages/appkit/src/plugins/serving/tests/serving.test.ts index a73e3be3..6f975025 100644 --- a/packages/appkit/src/plugins/serving/tests/serving.test.ts +++ b/packages/appkit/src/plugins/serving/tests/serving.test.ts @@ -349,7 +349,10 @@ describe("Serving Plugin", () => { "test-endpoint", { messages: [] }, ); - expect(result).toEqual({ choices: [{ message: { content: "Hi" } }] }); + expect(result).toEqual({ + ok: true, + data: { choices: [{ message: { content: "Hi" } }] }, + }); }); test("invoke throws for unknown alias", async () => {