From f08af8d87ddbc6f3d47e937d1e6469e35874f036 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Thu, 2 Apr 2026 20:14:42 +0200 Subject: [PATCH 1/9] refactor: replace undefined return with ExecutionResult in execute() Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .gitignore | 2 + packages/appkit/src/index.ts | 7 +- .../appkit/src/plugin/execution-result.ts | 9 +++ packages/appkit/src/plugin/index.ts | 1 + packages/appkit/src/plugin/plugin.ts | 38 ++++++++--- packages/appkit/src/plugins/files/plugin.ts | 68 +++++++++++-------- 6 files changed, 88 insertions(+), 37 deletions(-) create mode 100644 packages/appkit/src/plugin/execution-result.ts diff --git a/.gitignore b/.gitignore index 3b6cc969..2136927b 100644 --- a/.gitignore +++ b/.gitignore @@ -8,4 +8,6 @@ coverage *.tsbuildinfo +.claude/ralph-state.md + .turbo diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 8db7f1d7..bc0b4bbd 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 } from "./plugins"; // Registry types and utilities for plugin manifests export type { diff --git a/packages/appkit/src/plugin/execution-result.ts b/packages/appkit/src/plugin/execution-result.ts new file mode 100644 index 00000000..eda0b598 --- /dev/null +++ b/packages/appkit/src/plugin/execution-result.ts @@ -0,0 +1,9 @@ +/** + * Discriminated union for plugin execution results. + * + * Replaces the previous `T | undefined` return type on `execute()`, + * preserving the HTTP status code and message from the original error. + */ +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 36dab543..d4dea308 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"; @@ -435,15 +436,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 +460,30 @@ 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, + }; + } + + 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/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 98a5da72..e745b347 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -445,11 +445,13 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "List failed", plugin: this.name }); + if (!result.ok) { + res + .status(result.status) + .json({ error: "List failed", plugin: this.name }); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "List failed"); } @@ -481,11 +483,13 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "Read failed", plugin: this.name }); + if (!result.ok) { + res + .status(result.status) + .json({ error: "Read failed", plugin: this.name }); return; } - res.type("text/plain").send(result); + res.type("text/plain").send(result.data); } catch (error) { this._handleApiError(res, error, "Read failed"); } @@ -545,8 +549,10 @@ 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) { + res + .status(response.status) + .json({ error: `${label} failed`, plugin: this.name }); return; } @@ -575,9 +581,9 @@ 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); @@ -624,13 +630,13 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { + if (!result.ok) { res - .status(500) + .status(result.status) .json({ error: "Exists check failed", plugin: this.name }); return; } - res.json({ exists: result }); + res.json({ exists: result.data }); } catch (error) { this._handleApiError(res, error, "Exists check failed"); } @@ -662,13 +668,13 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { + if (!result.ok) { res - .status(500) + .status(result.status) .json({ error: "Metadata fetch failed", plugin: this.name }); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Metadata fetch failed"); } @@ -700,11 +706,13 @@ export class FilesPlugin extends Plugin { ]), ); - if (result === undefined) { - res.status(500).json({ error: "Preview failed", plugin: this.name }); + if (!result.ok) { + res + .status(result.status) + .json({ error: "Preview failed", plugin: this.name }); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Preview failed"); } @@ -791,7 +799,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 +807,14 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - res.status(500).json({ error: "Upload failed", plugin: this.name }); + res + .status(result.status) + .json({ error: "Upload failed", plugin: this.name }); 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 +861,14 @@ export class FilesPlugin extends Plugin { connector, ); - if (result === undefined) { + if (!result.ok) { res - .status(500) + .status(result.status) .json({ error: "Create directory failed", plugin: this.name }); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Create directory failed"); } @@ -898,12 +908,14 @@ export class FilesPlugin extends Plugin { connector, ); - if (result === undefined) { - res.status(500).json({ error: "Delete failed", plugin: this.name }); + if (!result.ok) { + res + .status(result.status) + .json({ error: "Delete failed", plugin: this.name }); return; } - res.json(result); + res.json(result.data); } catch (error) { this._handleApiError(res, error, "Delete failed"); } From 8b520ba69a76918d86c9a1f01d2f5da5a2a67cac Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Thu, 2 Apr 2026 20:16:27 +0200 Subject: [PATCH 2/9] test: update execute() tests for ExecutionResult and add AppKitError subclass coverage Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .gitignore | 2 - .../appkit/src/plugin/tests/plugin.test.ts | 200 +++++++++++++++--- 2 files changed, 173 insertions(+), 29 deletions(-) diff --git a/.gitignore b/.gitignore index 2136927b..3b6cc969 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,4 @@ coverage *.tsbuildinfo -.claude/ralph-state.md - .turbo diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index cc64e0a5..c7f30dd0 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,193 @@ 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 return error result for 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: 500, + message: expect.any(String), + }); }); - test("should swallow ApiError 401 and return undefined", async () => { + test("should return error result for ApiError 401 (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: 500, + message: expect.any(String), + }); }); - test("should swallow ApiError 403 and return undefined", async () => { + test("should return error result for ApiError 403 (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: 500, + message: expect.any(String), + }); + }); + + 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, options, false); - expect(result).toBeUndefined(); + 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 +850,7 @@ describe("Plugin", () => { user: { retry: { attempts: 3 } }, }); - expect(result).toBe("integration-result"); + expect(result).toEqual({ ok: true, data: "integration-result" }); }); }); }); From 1f5d80d3b3488bc6f8f50ef80ec2d7b7ab911ad0 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Thu, 2 Apr 2026 21:29:04 +0200 Subject: [PATCH 3/9] chore: docs update --- docs/docs/api/appkit/Class.Plugin.md | 12 ++++---- .../api/appkit/TypeAlias.ExecutionResult.md | 25 +++++++++++++++++ docs/docs/api/appkit/index.md | 1 + docs/docs/api/appkit/typedoc-sidebar.ts | 5 ++++ docs/static/appkit-ui/styles.gen.css | 28 +++++++++++++++++-- 5 files changed, 63 insertions(+), 8 deletions(-) create mode 100644 docs/docs/api/appkit/TypeAlias.ExecutionResult.md diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index 12a9c7bc..2cb5d73f 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..59f2bc29 --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md @@ -0,0 +1,25 @@ +# 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()`, +preserving the HTTP status code and message from the original error. + +## Type Parameters + +| Type Parameter | +| ------ | +| `T` | diff --git a/docs/docs/api/appkit/index.md b/docs/docs/api/appkit/index.md index b5fb7ce0..1805b966 100644 --- a/docs/docs/api/appkit/index.md +++ b/docs/docs/api/appkit/index.md @@ -51,6 +51,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 2f17b1d2..50c5e743 100644 --- a/docs/docs/api/appkit/typedoc-sidebar.ts +++ b/docs/docs/api/appkit/typedoc-sidebar.ts @@ -168,6 +168,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/docs/static/appkit-ui/styles.gen.css b/docs/static/appkit-ui/styles.gen.css index 9a9a38eb..a2192039 100644 --- a/docs/static/appkit-ui/styles.gen.css +++ b/docs/static/appkit-ui/styles.gen.css @@ -831,9 +831,6 @@ .max-w-\[calc\(100\%-2rem\)\] { max-width: calc(100% - 2rem); } - .max-w-full { - max-width: 100%; - } .max-w-max { max-width: max-content; } @@ -4514,6 +4511,11 @@ width: calc(var(--spacing) * 5); } } + .\[\&_\[data-slot\=scroll-area-viewport\]\>div\]\:\!block { + & [data-slot=scroll-area-viewport]>div { + display: block !important; + } + } .\[\&_a\]\:underline { & a { text-decoration-line: underline; @@ -4637,11 +4639,26 @@ color: var(--muted-foreground); } } + .\[\&_table\]\:block { + & table { + display: block; + } + } + .\[\&_table\]\:max-w-full { + & table { + max-width: 100%; + } + } .\[\&_table\]\:border-collapse { & table { border-collapse: collapse; } } + .\[\&_table\]\:overflow-x-auto { + & table { + overflow-x: auto; + } + } .\[\&_table\]\:text-xs { & table { font-size: var(--text-xs); @@ -4851,6 +4868,11 @@ width: 100%; } } + .\[\&\>\*\]\:min-w-0 { + &>* { + min-width: calc(var(--spacing) * 0); + } + } .\[\&\>\*\]\:focus-visible\:relative { &>* { &:focus-visible { From 3a0adad7a340053d083313c6540db2fe2b72cacd Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 21:22:48 +0200 Subject: [PATCH 4/9] chore: preserve upstream HTTP status codes in execute() for non-AppKitError errors --- .../appkit/src/plugin/execution-result.ts | 12 ++- packages/appkit/src/plugin/plugin.ts | 24 ++++++ .../appkit/src/plugin/tests/plugin.test.ts | 81 ++++++++++++++++--- .../files/tests/plugin.integration.test.ts | 8 +- 4 files changed, 110 insertions(+), 15 deletions(-) diff --git a/packages/appkit/src/plugin/execution-result.ts b/packages/appkit/src/plugin/execution-result.ts index eda0b598..964e8983 100644 --- a/packages/appkit/src/plugin/execution-result.ts +++ b/packages/appkit/src/plugin/execution-result.ts @@ -1,8 +1,16 @@ /** * Discriminated union for plugin execution results. * - * Replaces the previous `T | undefined` return type on `execute()`, - * preserving the HTTP status code and message from the original error. + * 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 } diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index d4dea308..b27862e6 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -41,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 @@ -477,6 +491,16 @@ export abstract class Plugin< }; } + 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, diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index c7f30dd0..440579d7 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -387,7 +387,7 @@ describe("Plugin", () => { } }); - test("should return error result for ApiError (non-AppKitError)", 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); @@ -399,12 +399,12 @@ describe("Plugin", () => { ); expect(result).toEqual({ ok: false, - status: 500, - message: expect.any(String), + status: 404, + message: "Not found", }); }); - test("should return error result for ApiError 401 (non-AppKitError)", 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); @@ -416,12 +416,12 @@ describe("Plugin", () => { ); expect(result).toEqual({ ok: false, - status: 500, - message: expect.any(String), + status: 401, + message: "Unauthorized", }); }); - test("should return error result for ApiError 403 (non-AppKitError)", 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); @@ -433,11 +433,74 @@ describe("Plugin", () => { ); expect(result).toEqual({ ok: false, - status: 500, - message: expect.any(String), + status: 403, + message: "Forbidden", }); }); + 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 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..de03978d 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -587,7 +587,7 @@ describe("Files Plugin Integration", () => { 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,7 +597,7 @@ 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; @@ -606,7 +606,7 @@ describe("Files Plugin Integration", () => { 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,7 +617,7 @@ 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; From d3ac0cff79c40ccad73fd92e4cfda6c23a3d4555 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 23:42:37 +0200 Subject: [PATCH 5/9] docs: regenerate ExecutionResult API docs with error handling details Co-authored-by: Isaac Signed-off-by: Atila Fassina --- docs/docs/api/appkit/TypeAlias.ExecutionResult.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md index 59f2bc29..1af9686e 100644 --- a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md +++ b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md @@ -15,8 +15,16 @@ type ExecutionResult = Discriminated union for plugin execution results. -Replaces the previous `T | undefined` return type on `execute()`, -preserving the HTTP status code and message from the original error. +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 From 8be15c73cfb36dcee9f818f0792df1713f807b35 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 8 Apr 2026 12:50:11 +0200 Subject: [PATCH 6/9] refactor: use HTTP status code descriptions in files plugin (#258) * refactor: use HTTP status code descriptions instead of hardcoded error strings Replace hardcoded error messages like "List failed" with standard HTTP status descriptions from node:http STATUS_CODES for consistent error responses. Co-authored-by: Isaac Signed-off-by: Atila Fassina * chore: dry status error responses in files plugin Co-authored-by: Isaac Signed-off-by: Atila Fassina * fix: standardize stream error to use _sendStatusError helper Addresses Copilot review comment about mixed error formats in the read/download stream handler. Co-authored-by: Isaac Signed-off-by: Atila Fassina --------- Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/files/plugin.ts | 48 +++++++------------ .../files/tests/plugin.integration.test.ts | 8 ++-- .../src/plugins/files/tests/plugin.test.ts | 16 +++---- 3 files changed, 30 insertions(+), 42 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index e745b347..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, @@ -446,9 +454,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "List failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -484,9 +490,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Read failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } res.type("text/plain").send(result.data); @@ -550,9 +554,7 @@ export class FilesPlugin extends Plugin { }, settings); if (!response.ok) { - res - .status(response.status) - .json({ error: `${label} failed`, plugin: this.name }); + this._sendStatusError(res, response.status); return; } @@ -588,9 +590,7 @@ export class FilesPlugin extends Plugin { 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(); } @@ -631,9 +631,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Exists check failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } res.json({ exists: result.data }); @@ -669,9 +667,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Metadata fetch failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -707,9 +703,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Preview failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -807,9 +801,7 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - res - .status(result.status) - .json({ error: "Upload failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } @@ -862,9 +854,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Create directory failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } @@ -909,9 +899,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ error: "Delete failed", plugin: this.name }); + this._sendStatusError(res, result.status); return; } 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 de03978d..8307dcc1 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,7 +583,7 @@ 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"); }); @@ -602,7 +602,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Metadata fetch failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); @@ -622,7 +622,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Create directory failed"); + expect(data.error).toBe("Internal Server Error"); 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" }), ); }); From 7032f72345684d48e2d44b659e2cd56013744d93 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 8 Apr 2026 13:25:54 +0200 Subject: [PATCH 7/9] fix: correct test assertions for 404/409 status text in files plugin The _sendStatusError() helper returns STATUS_CODES[status] (e.g., "Not Found" for 404, "Conflict" for 409), but tests incorrectly expected "Internal Server Error" for all error status codes. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .../appkit/src/plugins/files/tests/plugin.integration.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 8307dcc1..55989148 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -602,7 +602,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Internal Server Error"); + expect(data.error).toBe("Not Found"); expect(data.plugin).toBe("files"); }); @@ -622,7 +622,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Internal Server Error"); + expect(data.error).toBe("Conflict"); expect(data.plugin).toBe("files"); }); }); From c92dc8e4fc48d060558bc466eb271c4756f6c140 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 18:11:41 +0200 Subject: [PATCH 8/9] chore: fix unit tests --- packages/appkit/src/plugins/serving/serving.ts | 6 +++--- packages/appkit/src/plugins/serving/tests/serving.test.ts | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/appkit/src/plugins/serving/serving.ts b/packages/appkit/src/plugins/serving/serving.ts index f64f6c95..614c977b 100644 --- a/packages/appkit/src/plugins/serving/serving.ts +++ b/packages/appkit/src/plugins/serving/serving.ts @@ -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) { 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 () => { From 322e6125958119e82ee8683eae8e40e85a1285a1 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 18:24:17 +0200 Subject: [PATCH 9/9] fix: type serving invoke() return as ExecutionResult Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/serving/serving.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/appkit/src/plugins/serving/serving.ts b/packages/appkit/src/plugins/serving/serving.ts index 614c977b..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"; @@ -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;