From 8754b32139134263bc86ebcc4368229dbd7c519f Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 11 Jun 2026 18:27:47 +0200 Subject: [PATCH 1/6] fix(server): add terminal error middleware so async handler rejections return JSON errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route handlers registered via Plugin.route() are async, but Express 4 does not catch rejected promises from async handlers. When a handler rejected before writing a response — e.g. asUser(req) throwing AuthenticationError because the x-forwarded-access-token / x-forwarded-user headers were missing on genie/serving routes — the rejection escaped Express entirely, the request hung with no response, and the unhandledRejection crashed the Node process (default behavior since Node 15). Any direct hit without proxy headers (scanner, curl, proxy misconfig) could take the app down. Fix: - Plugin.route() now wraps handlers to catch sync throws and async rejections and forward them to Express via next(err). - ServerPlugin registers a terminal (err, req, res, next) middleware after all routes, extensions, and frontend middleware. It logs the error, guards res.headersSent, maps AppKitError to its statusCode with its message, preserves HTTP statusCode-bearing errors (masking 5xx messages in production), and maps unknown errors to 500 with a generic message in production — matching Plugin.execute() semantics. Adds unit tests for the middleware and integration tests asserting a handler that throws AuthenticationError returns 401 JSON and a generic throw returns 500 JSON, with no unhandledRejection in either case. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- packages/appkit/src/plugin/plugin.ts | 15 +- packages/appkit/src/plugins/server/index.ts | 74 +++++++++- .../server/tests/error-middleware.test.ts | 128 ++++++++++++++++++ .../server/tests/server.integration.test.ts | 125 ++++++++++++++++- 4 files changed, 339 insertions(+), 3 deletions(-) create mode 100644 packages/appkit/src/plugins/server/tests/error-middleware.test.ts diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index c8138fd39..ed37a137f 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -666,7 +666,20 @@ export abstract class Plugin< ): void { const { name, method, path, handler } = config; - router[method](path, handler); + // Express 4 does not catch rejected promises from async handlers — a + // rejection (e.g. asUser() throwing AuthenticationError before any + // response is written) escapes as an unhandledRejection, which leaves + // the request hanging and crashes Node by default. Catch both sync + // throws and async rejections and forward them to the Express error + // middleware via next(err). + // The promise chain is returned so callers that invoke the handler + // directly (e.g. unit tests) can await completion; Express ignores it. + const safeHandler: express.RequestHandler = (req, res, next) => + Promise.resolve() + .then(() => handler(req, res)) + .catch(next); + + router[method](path, safeHandler); const fullPath = `/api/${this.name}${path}`; this.registerEndpoint(name, fullPath); diff --git a/packages/appkit/src/plugins/server/index.ts b/packages/appkit/src/plugins/server/index.ts index e66abf5ad..07a5e9c41 100644 --- a/packages/appkit/src/plugins/server/index.ts +++ b/packages/appkit/src/plugins/server/index.ts @@ -5,7 +5,7 @@ import dotenv from "dotenv"; import express from "express"; import getPort, { portNumbers } from "get-port"; import type { PluginClientConfigs, PluginPhase } from "shared"; -import { ServerError } from "../../errors"; +import { AppKitError, ServerError } from "../../errors"; import { TelemetryReporter } from "../../internal-telemetry"; import { createLogger } from "../../logging/logger"; import { Plugin, toPlugin } from "../../plugin"; @@ -147,6 +147,12 @@ export class ServerPlugin extends Plugin { await this.setupFrontend(endpoints, pluginConfigs); + // Terminal error handler — registered after all routes, extensions, and + // frontend middleware so that errors forwarded via next(err) from any of + // them (e.g. async handler rejections forwarded by Plugin.route()) are + // turned into JSON error responses instead of hanging the request. + this.serverApplication.use(errorHandlerMiddleware); + const listenPort = await this.resolveListenPort(); const server = this.serverApplication.listen( @@ -505,6 +511,72 @@ export function requestMetricsMiddleware( next(); } +/** + * Narrow an unknown thrown value to an Error that carries a numeric + * `statusCode` property in the HTTP error range (e.g. `ApiError` from + * `@databricks/sdk-experimental`). + */ +function hasHttpStatusCode( + error: unknown, +): error is Error & { statusCode: number } { + if (!(error instanceof Error) || !("statusCode" in error)) return false; + const statusCode = (error as Record).statusCode; + return ( + typeof statusCode === "number" && statusCode >= 400 && statusCode <= 599 + ); +} + +/** + * Terminal Express error-handling middleware. + * + * Converts errors forwarded via `next(err)` (including async handler + * rejections forwarded by `Plugin.route()`) into JSON error responses, + * following the same status/message conventions as `Plugin.execute()`: + * - `AppKitError` → its `statusCode` with its message + * - errors with an HTTP `statusCode` → that status; 5xx messages are + * masked in production to avoid leaking internals + * - anything else → 500 with a generic message in production + * + * @internal Exported for unit tests. + */ +export function errorHandlerMiddleware( + err: unknown, + req: express.Request, + res: express.Response, + next: express.NextFunction, +) { + logger.error( + "Unhandled error for %s %s: %O", + req.method, + req.originalUrl, + err, + ); + + if (res.headersSent) { + next(err); + return; + } + + if (err instanceof AppKitError) { + res.status(err.statusCode).json({ error: err.message }); + return; + } + + const isDev = process.env.NODE_ENV !== "production"; + + if (hasHttpStatusCode(err)) { + const isClientError = err.statusCode < 500; + res.status(err.statusCode).json({ + error: isDev || isClientError ? err.message : "Server error", + }); + return; + } + + res.status(500).json({ + error: isDev && err instanceof Error ? err.message : "Server error", + }); +} + /** * @internal */ diff --git a/packages/appkit/src/plugins/server/tests/error-middleware.test.ts b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts new file mode 100644 index 000000000..02851137d --- /dev/null +++ b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts @@ -0,0 +1,128 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { AuthenticationError, ValidationError } from "../../../errors"; +import { errorHandlerMiddleware } from "../index"; + +function makeReq() { + return { method: "GET", originalUrl: "/api/genie/default/messages" } as any; +} + +function makeRes(headersSent = false) { + const res: any = { headersSent }; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + return res; +} + +describe("errorHandlerMiddleware", () => { + const originalNodeEnv = process.env.NODE_ENV; + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + vi.restoreAllMocks(); + }); + + test("maps AppKitError to its statusCode with its message", () => { + const res = makeRes(); + const next = vi.fn(); + + errorHandlerMiddleware( + AuthenticationError.missingToken("user token"), + makeReq(), + res, + next, + ); + + expect(res.status).toHaveBeenCalledWith(401); + expect(res.json).toHaveBeenCalledWith({ + error: "Missing user token in request headers", + }); + expect(next).not.toHaveBeenCalled(); + }); + + test("preserves AppKitError messages in production (client-safe by design)", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new ValidationError("content is required"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ error: "content is required" }); + }); + + test("maps unknown errors to 500 with the message outside production", () => { + const res = makeRes(); + + errorHandlerMiddleware(new Error("boom"), makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "boom" }); + }); + + test("masks unknown error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new Error("internal details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("preserves statusCode-bearing 4xx error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + const err = Object.assign(new Error("resource not found"), { + statusCode: 404, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: "resource not found" }); + }); + + test("masks statusCode-bearing 5xx error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + const err = Object.assign(new Error("upstream exploded"), { + statusCode: 502, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(502); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("delegates to next(err) when headers are already sent", () => { + const res = makeRes(true); + const next = vi.fn(); + const err = new Error("late failure"); + + errorHandlerMiddleware(err, makeReq(), res, next); + + expect(next).toHaveBeenCalledWith(err); + expect(res.status).not.toHaveBeenCalled(); + expect(res.json).not.toHaveBeenCalled(); + }); + + test("handles non-Error thrown values with a generic 500", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware("string error", makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); +}); diff --git a/packages/appkit/src/plugins/server/tests/server.integration.test.ts b/packages/appkit/src/plugins/server/tests/server.integration.test.ts index 0b67e4c06..5ed0190a7 100644 --- a/packages/appkit/src/plugins/server/tests/server.integration.test.ts +++ b/packages/appkit/src/plugins/server/tests/server.integration.test.ts @@ -6,9 +6,10 @@ import { afterAll, beforeAll, describe, expect, test } from "vitest"; process.env.DATABRICKS_APP_PORT = "8000"; process.env.FLASK_RUN_HOST = "0.0.0.0"; -import type { PluginManifest } from "shared"; +import type { IAppResponse, IAppRouter, PluginManifest } from "shared"; import { ServiceContext } from "../../../context/service-context"; import { createApp } from "../../../core"; +import { AuthenticationError } from "../../../errors"; import { Plugin, toPlugin } from "../../../plugin"; import { server as serverPlugin } from "../index"; @@ -278,6 +279,128 @@ describe("createApp with async onPluginsReady callback", () => { }); }); +describe("ServerPlugin error handling for rejected async handlers", () => { + let server: Server; + let baseUrl: string; + let serviceContextMock: Awaited>; + const TEST_PORT = 9879; + const unhandledRejections: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => { + unhandledRejections.push(reason); + }; + + beforeAll(async () => { + setupDatabricksEnv(); + ServiceContext.reset(); + serviceContextMock = await mockServiceContext(); + process.on("unhandledRejection", onUnhandledRejection); + + // Plugin whose handlers reject before writing a response — mirrors the + // genie/serving pattern of `await this.asUser(req)._handleX(req, res)` + // hit without forwarded auth headers. + class ThrowingPlugin extends Plugin { + static manifest = { + name: "throwing-plugin", + displayName: "Throwing Plugin", + description: "Test plugin whose handlers reject", + resources: { required: [], optional: [] }, + } satisfies PluginManifest<"throwing-plugin">; + + injectRoutes(router: IAppRouter) { + this.route(router, { + name: "authError", + method: "get", + path: "/auth-error", + handler: async (req, res) => { + // asUser() throws AuthenticationError when the request has no + // x-forwarded-access-token header (outside development mode). + await this.asUser(req)._whoAmI(res); + }, + }); + + this.route(router, { + name: "genericError", + method: "get", + path: "/generic-error", + handler: async () => { + throw new Error("boom"); + }, + }); + } + + async _whoAmI(res: IAppResponse): Promise { + res.json({ ok: true }); + } + } + + const throwingPlugin = toPlugin(ThrowingPlugin); + + const app = await createApp({ + plugins: [ + serverPlugin({ + port: TEST_PORT, + host: "127.0.0.1", + }), + throwingPlugin({}), + ], + }); + + server = app.server.getServer(); + baseUrl = `http://127.0.0.1:${TEST_PORT}`; + + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + + afterAll(async () => { + process.off("unhandledRejection", onUnhandledRejection); + serviceContextMock?.restore(); + if (server) { + await new Promise((resolve, reject) => { + server.close((err) => { + if (err) reject(err); + else resolve(); + }); + }); + } + }); + + test("handler throwing AuthenticationError returns 401 JSON without unhandled rejection", async () => { + const response = await fetch( + `${baseUrl}/api/throwing-plugin/auth-error`, + // No x-forwarded-access-token / x-forwarded-user headers. + ); + + expect(response.status).toBe(401); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe( + AuthenticationError.missingToken("user token").message, + ); + + // Give a potential unhandledRejection a chance to fire. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); + + test("handler throwing a generic error returns 500 JSON without unhandled rejection", async () => { + const response = await fetch( + `${baseUrl}/api/throwing-plugin/generic-error`, + ); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + // NODE_ENV is not "production" under vitest, so the original message + // is surfaced; in production it would be masked as "Server error". + expect(data.error).toBe("boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); +}); + describe("createApp without server plugin", () => { let serviceContextMock: Awaited>; let onPluginsReadyWasCalled = false; From c277d7039d6375afd769909b714fb89a93fc7373 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 11 Jun 2026 19:14:07 +0200 Subject: [PATCH 2/6] fix(appkit): wrap buffered route handlers and align error middleware logging/masking Follow-up to the terminal error middleware, addressing review findings: - Factor the async-rejection wrapper into forwardAsyncErrors (utils/ safe-handler.ts) and use it from both Plugin.route() and PluginContext applyRoute/applyMiddleware, so handlers registered through the buffering APIs no longer escape Express 4 as unhandledRejections. 4-arg error middleware is passed through unwrapped (Express detects it by arity). extend() JSDoc now states that async handlers registered directly on the app must handle their own rejections. - errorHandlerMiddleware resolves the status first and logs 4xx at warn with just the message (expected client errors), 5xx/unknown at error with the full error object. - 5xx AppKitError messages are masked as "Server error" outside development, mirroring Plugin.execute(); 4xx messages stay as-is. - hasHttpStatusCode now also requires Number.isInteger(statusCode). - Tests: pin NODE_ENV in the integration suite and scope its unhandledRejection listener to this suite's errors; add coverage for buffered-route rejections, log-level branching, and 5xx masking. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- packages/appkit/src/core/plugin-context.ts | 9 +- .../src/core/tests/plugin-context.test.ts | 76 +++++++++++-- packages/appkit/src/plugin/plugin.ts | 18 +--- packages/appkit/src/plugins/server/index.ts | 60 +++++++---- .../server/tests/error-middleware.test.ts | 101 +++++++++++++++++- .../server/tests/server.integration.test.ts | 48 ++++++++- packages/appkit/src/utils/safe-handler.ts | 28 +++++ 7 files changed, 291 insertions(+), 49 deletions(-) create mode 100644 packages/appkit/src/utils/safe-handler.ts diff --git a/packages/appkit/src/core/plugin-context.ts b/packages/appkit/src/core/plugin-context.ts index eda3f05a5..cae027284 100644 --- a/packages/appkit/src/core/plugin-context.ts +++ b/packages/appkit/src/core/plugin-context.ts @@ -2,6 +2,7 @@ import type express from "express"; import type { BasePlugin, IAppRequest, ToolProvider } from "shared"; import { createLogger } from "../logging/logger"; import { SpanStatusCode, TelemetryManager } from "../telemetry"; +import { forwardAsyncErrors } from "../utils/safe-handler"; const logger = createLogger("plugin-context"); @@ -287,7 +288,9 @@ export class PluginContext { if (typeof app[method] === "function") { (app[method] as (...a: unknown[]) => void)( route.path, - ...route.handlers, + // Forward sync throws and async rejections to the Express error + // middleware — Express 4 does not do this on its own. + ...route.handlers.map(forwardAsyncErrors), ); } }); @@ -299,7 +302,9 @@ export class PluginContext { ): void { if (!this.routeTarget) return; this.routeTarget.addExtension((app) => { - app.use(path, ...handlers); + // forwardAsyncErrors leaves 4-arg error middleware untouched, so error + // handlers passed through addMiddleware keep their Express semantics. + app.use(path, ...handlers.map(forwardAsyncErrors)); }); } } diff --git a/packages/appkit/src/core/tests/plugin-context.test.ts b/packages/appkit/src/core/tests/plugin-context.test.ts index 868cb183f..3f548ec57 100644 --- a/packages/appkit/src/core/tests/plugin-context.test.ts +++ b/packages/appkit/src/core/tests/plugin-context.test.ts @@ -75,7 +75,7 @@ describe("PluginContext", () => { expect(ctx.getPluginNames()).toEqual([]); }); - test("flushRoutes applies buffered routes via addExtension", () => { + test("flushRoutes applies buffered routes via addExtension", async () => { const handler = vi.fn(); ctx.addRoute("post", "/invocations", handler); @@ -87,10 +87,19 @@ describe("PluginContext", () => { const mockApp = { post: vi.fn() }; extensionFn(mockApp); - expect(mockApp.post).toHaveBeenCalledWith("/invocations", handler); + // Handlers are wrapped (forwardAsyncErrors) so async rejections reach + // the Express error middleware — assert the wrapper delegates. + expect(mockApp.post).toHaveBeenCalledWith( + "/invocations", + expect.any(Function), + ); + const wrapped = mockApp.post.mock.calls[0][1]; + const next = vi.fn(); + await wrapped("req", "res", next); + expect(handler).toHaveBeenCalledWith("req", "res", next); }); - test("addRoute called after registerAsRouteTarget applies immediately", () => { + test("addRoute called after registerAsRouteTarget applies immediately", async () => { const addExtension = vi.fn(); ctx.registerAsRouteTarget({ addExtension }); @@ -102,10 +111,13 @@ describe("PluginContext", () => { const mockApp = { get: vi.fn() }; extensionFn(mockApp); - expect(mockApp.get).toHaveBeenCalledWith("/health", handler); + expect(mockApp.get).toHaveBeenCalledWith("/health", expect.any(Function)); + const wrapped = mockApp.get.mock.calls[0][1]; + await wrapped("req", "res", vi.fn()); + expect(handler).toHaveBeenCalled(); }); - test("addRoute supports middleware chains", () => { + test("addRoute supports middleware chains", async () => { const auth = vi.fn(); const handler = vi.fn(); @@ -117,10 +129,19 @@ describe("PluginContext", () => { const extensionFn = addExtension.mock.calls[0][0]; const mockApp = { post: vi.fn() }; extensionFn(mockApp); - expect(mockApp.post).toHaveBeenCalledWith("/api", auth, handler); + expect(mockApp.post).toHaveBeenCalledWith( + "/api", + expect.any(Function), + expect.any(Function), + ); + const [, wrappedAuth, wrappedHandler] = mockApp.post.mock.calls[0]; + await wrappedAuth("req", "res", vi.fn()); + await wrappedHandler("req", "res", vi.fn()); + expect(auth).toHaveBeenCalled(); + expect(handler).toHaveBeenCalled(); }); - test("addMiddleware buffers and applies via use()", () => { + test("addMiddleware buffers and applies via use()", async () => { const handler = vi.fn(); ctx.addMiddleware("/api", handler); @@ -132,7 +153,46 @@ describe("PluginContext", () => { const mockApp = { use: vi.fn() }; extensionFn(mockApp); - expect(mockApp.use).toHaveBeenCalledWith("/api", handler); + expect(mockApp.use).toHaveBeenCalledWith("/api", expect.any(Function)); + const wrapped = mockApp.use.mock.calls[0][1]; + await wrapped("req", "res", vi.fn()); + expect(handler).toHaveBeenCalled(); + }); + + test("wrapped route handlers forward async rejections to next", async () => { + const failure = new Error("handler rejected"); + const handler = vi.fn().mockRejectedValue(failure); + const addExtension = vi.fn(); + ctx.registerAsRouteTarget({ addExtension }); + + ctx.addRoute("get", "/boom", handler); + + const extensionFn = addExtension.mock.calls[0][0]; + const mockApp = { get: vi.fn() }; + extensionFn(mockApp); + const wrapped = mockApp.get.mock.calls[0][1]; + const next = vi.fn(); + await wrapped("req", "res", next); + expect(next).toHaveBeenCalledWith(failure); + }); + + test("addMiddleware leaves 4-arg error middleware unwrapped", () => { + const errorMiddleware = ( + _err: unknown, + _req: unknown, + _res: unknown, + _next: unknown, + ) => {}; + const addExtension = vi.fn(); + ctx.registerAsRouteTarget({ addExtension }); + + ctx.addMiddleware("/api", errorMiddleware as never); + + const extensionFn = addExtension.mock.calls[0][0]; + const mockApp = { use: vi.fn() }; + extensionFn(mockApp); + // Express detects error middleware by arity — wrapping would break it. + expect(mockApp.use).toHaveBeenCalledWith("/api", errorMiddleware); }); test("multiple buffered routes are all applied on registration", () => { diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index ed37a137f..17030125e 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -25,6 +25,7 @@ import { TelemetryManager, } from "../telemetry"; import { deepMerge } from "../utils"; +import { forwardAsyncErrors } from "../utils/safe-handler"; import { DevFileReader } from "./dev-reader"; import type { ExecutionResult } from "./execution-result"; import { CacheInterceptor } from "./interceptors/cache"; @@ -666,20 +667,9 @@ export abstract class Plugin< ): void { const { name, method, path, handler } = config; - // Express 4 does not catch rejected promises from async handlers — a - // rejection (e.g. asUser() throwing AuthenticationError before any - // response is written) escapes as an unhandledRejection, which leaves - // the request hanging and crashes Node by default. Catch both sync - // throws and async rejections and forward them to the Express error - // middleware via next(err). - // The promise chain is returned so callers that invoke the handler - // directly (e.g. unit tests) can await completion; Express ignores it. - const safeHandler: express.RequestHandler = (req, res, next) => - Promise.resolve() - .then(() => handler(req, res)) - .catch(next); - - router[method](path, safeHandler); + // Sync throws and async rejections are forwarded to the Express error + // middleware via next(err) — Express 4 does not do this on its own. + router[method](path, forwardAsyncErrors(handler)); const fullPath = `/api/${this.name}${path}`; this.registerEndpoint(name, fullPath); diff --git a/packages/appkit/src/plugins/server/index.ts b/packages/appkit/src/plugins/server/index.ts index 07a5e9c41..ea878058d 100644 --- a/packages/appkit/src/plugins/server/index.ts +++ b/packages/appkit/src/plugins/server/index.ts @@ -198,6 +198,11 @@ export class ServerPlugin extends Plugin { * Call this inside the `onPluginsReady` callback of `createApp` to register * custom Express routes or middleware before the server starts listening. * + * Note: async handlers registered directly on the app must handle their own + * rejections — Express 4 does not forward rejected promises to the error + * middleware. Errors passed explicitly to `next(err)` are formatted by the + * terminal error middleware. + * * @param fn - A function that receives the express application. * @returns The server plugin instance for chaining. */ @@ -522,7 +527,10 @@ function hasHttpStatusCode( if (!(error instanceof Error) || !("statusCode" in error)) return false; const statusCode = (error as Record).statusCode; return ( - typeof statusCode === "number" && statusCode >= 400 && statusCode <= 599 + typeof statusCode === "number" && + Number.isInteger(statusCode) && + statusCode >= 400 && + statusCode <= 599 ); } @@ -532,11 +540,14 @@ function hasHttpStatusCode( * Converts errors forwarded via `next(err)` (including async handler * rejections forwarded by `Plugin.route()`) into JSON error responses, * following the same status/message conventions as `Plugin.execute()`: - * - `AppKitError` → its `statusCode` with its message - * - errors with an HTTP `statusCode` → that status; 5xx messages are - * masked in production to avoid leaking internals + * - errors carrying an HTTP `statusCode` (including `AppKitError`) → that + * status; 5xx messages are masked in production to avoid leaking internals * - anything else → 500 with a generic message in production * + * 4xx errors are expected client errors (e.g. missing auth headers) and are + * logged at `warn` with just the message; 5xx/unknown errors are logged at + * `error` with the full error. + * * @internal Exported for unit tests. */ export function errorHandlerMiddleware( @@ -545,34 +556,43 @@ export function errorHandlerMiddleware( res: express.Response, next: express.NextFunction, ) { - logger.error( - "Unhandled error for %s %s: %O", - req.method, - req.originalUrl, - err, - ); + const httpError = + err instanceof AppKitError || hasHttpStatusCode(err) ? err : null; + const statusCode = httpError ? httpError.statusCode : 500; + const isClientError = statusCode < 500; + + if (isClientError) { + logger.warn( + "Request failed for %s %s with status %d: %s", + req.method, + req.originalUrl, + statusCode, + err instanceof Error ? err.message : String(err), + ); + } else { + logger.error( + "Unhandled error for %s %s: %O", + req.method, + req.originalUrl, + err, + ); + } if (res.headersSent) { next(err); return; } - if (err instanceof AppKitError) { - res.status(err.statusCode).json({ error: err.message }); - return; - } - const isDev = process.env.NODE_ENV !== "production"; - if (hasHttpStatusCode(err)) { - const isClientError = err.statusCode < 500; - res.status(err.statusCode).json({ - error: isDev || isClientError ? err.message : "Server error", + if (httpError) { + res.status(statusCode).json({ + error: isDev || isClientError ? httpError.message : "Server error", }); return; } - res.status(500).json({ + res.status(statusCode).json({ error: isDev && err instanceof Error ? err.message : "Server error", }); } diff --git a/packages/appkit/src/plugins/server/tests/error-middleware.test.ts b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts index 02851137d..9742e07e3 100644 --- a/packages/appkit/src/plugins/server/tests/error-middleware.test.ts +++ b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts @@ -1,7 +1,25 @@ import { afterEach, describe, expect, test, vi } from "vitest"; -import { AuthenticationError, ValidationError } from "../../../errors"; +import { + AuthenticationError, + ServerError, + ValidationError, +} from "../../../errors"; import { errorHandlerMiddleware } from "../index"; +const loggerSpies = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + event: vi.fn(), +})); + +vi.mock("../../../logging/logger", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, createLogger: () => loggerSpies }; +}); + function makeReq() { return { method: "GET", originalUrl: "/api/genie/default/messages" } as any; } @@ -18,7 +36,7 @@ describe("errorHandlerMiddleware", () => { afterEach(() => { process.env.NODE_ENV = originalNodeEnv; - vi.restoreAllMocks(); + vi.clearAllMocks(); }); test("maps AppKitError to its statusCode with its message", () => { @@ -39,7 +57,7 @@ describe("errorHandlerMiddleware", () => { expect(next).not.toHaveBeenCalled(); }); - test("preserves AppKitError messages in production (client-safe by design)", () => { + test("preserves 4xx AppKitError messages in production (client-safe by design)", () => { process.env.NODE_ENV = "production"; const res = makeRes(); @@ -54,6 +72,38 @@ describe("errorHandlerMiddleware", () => { expect(res.json).toHaveBeenCalledWith({ error: "content is required" }); }); + test("masks 5xx AppKitError messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new ServerError("internal connection details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("preserves 5xx AppKitError messages outside production", () => { + process.env.NODE_ENV = "test"; + const res = makeRes(); + + errorHandlerMiddleware( + new ServerError("internal connection details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ + error: "internal connection details", + }); + }); + test("maps unknown errors to 500 with the message outside production", () => { const res = makeRes(); @@ -104,6 +154,51 @@ describe("errorHandlerMiddleware", () => { expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); }); + test("ignores non-integer statusCode values and falls back to 500", () => { + const res = makeRes(); + const err = Object.assign(new Error("weird status"), { + statusCode: 404.5, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "weird status" }); + }); + + test("logs 4xx errors at warn with the message, not the full error", () => { + const res = makeRes(); + const err = AuthenticationError.missingToken("user token"); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(loggerSpies.warn).toHaveBeenCalledTimes(1); + expect(loggerSpies.warn).toHaveBeenCalledWith( + expect.any(String), + "GET", + "/api/genie/default/messages", + 401, + err.message, + ); + expect(loggerSpies.error).not.toHaveBeenCalled(); + }); + + test("logs 5xx and unknown errors at error with the error object", () => { + const res = makeRes(); + const err = new Error("boom"); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(loggerSpies.error).toHaveBeenCalledTimes(1); + expect(loggerSpies.error).toHaveBeenCalledWith( + expect.any(String), + "GET", + "/api/genie/default/messages", + err, + ); + expect(loggerSpies.warn).not.toHaveBeenCalled(); + }); + test("delegates to next(err) when headers are already sent", () => { const res = makeRes(true); const next = vi.fn(); diff --git a/packages/appkit/src/plugins/server/tests/server.integration.test.ts b/packages/appkit/src/plugins/server/tests/server.integration.test.ts index 5ed0190a7..0553817c1 100644 --- a/packages/appkit/src/plugins/server/tests/server.integration.test.ts +++ b/packages/appkit/src/plugins/server/tests/server.integration.test.ts @@ -283,13 +283,29 @@ describe("ServerPlugin error handling for rejected async handlers", () => { let server: Server; let baseUrl: string; let serviceContextMock: Awaited>; + let originalNodeEnv: string | undefined; const TEST_PORT = 9879; const unhandledRejections: unknown[] = []; + // Only count rejections raised by this suite's handlers — other suites in + // the same worker may legitimately produce unrelated rejections. + const suiteErrorMessages = new Set([ + "boom", + "buffered boom", + AuthenticationError.missingToken("user token").message, + ]); const onUnhandledRejection = (reason: unknown) => { - unhandledRejections.push(reason); + if (reason instanceof Error && suiteErrorMessages.has(reason.message)) { + unhandledRejections.push(reason); + } }; beforeAll(async () => { + // The 401 test requires non-development (asUser must throw instead of + // falling back) and the "boom" assertion requires non-production (the + // message must not be masked) — pin NODE_ENV so both are deterministic. + originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "test"; + setupDatabricksEnv(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); @@ -306,6 +322,14 @@ describe("ServerPlugin error handling for rejected async handlers", () => { resources: { required: [], optional: [] }, } satisfies PluginManifest<"throwing-plugin">; + async setup() { + // Registered through PluginContext's buffering API — exercises the + // applyRoute path, which must wrap handlers like Plugin.route() does. + this.context?.addRoute("get", "/buffered-error", async () => { + throw new Error("buffered boom"); + }); + } + injectRoutes(router: IAppRouter) { this.route(router, { name: "authError", @@ -352,6 +376,11 @@ describe("ServerPlugin error handling for rejected async handlers", () => { }); afterAll(async () => { + if (originalNodeEnv === undefined) { + delete process.env.NODE_ENV; + } else { + process.env.NODE_ENV = originalNodeEnv; + } process.off("unhandledRejection", onUnhandledRejection); serviceContextMock?.restore(); if (server) { @@ -392,13 +421,28 @@ describe("ServerPlugin error handling for rejected async handlers", () => { expect(response.headers.get("content-type")).toContain("application/json"); const data = (await response.json()) as { error: string }; - // NODE_ENV is not "production" under vitest, so the original message + // NODE_ENV is pinned to "test" in beforeAll, so the original message // is surfaced; in production it would be masked as "Server error". expect(data.error).toBe("boom"); await new Promise((resolve) => setTimeout(resolve, 50)); expect(unhandledRejections).toEqual([]); }); + + test("buffered route with rejecting async handler returns 500 JSON without unhandled rejection", async () => { + // /buffered-error is mounted on the root app via PluginContext.addRoute, + // not via Plugin.route() — its handler must be wrapped by applyRoute. + const response = await fetch(`${baseUrl}/buffered-error`); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe("buffered boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); }); describe("createApp without server plugin", () => { diff --git a/packages/appkit/src/utils/safe-handler.ts b/packages/appkit/src/utils/safe-handler.ts new file mode 100644 index 000000000..541fb50e0 --- /dev/null +++ b/packages/appkit/src/utils/safe-handler.ts @@ -0,0 +1,28 @@ +import type express from "express"; + +/** + * Wrap an Express handler so synchronous throws and async rejections are + * forwarded to the error-handling middleware via `next(err)`. + * + * Express 4 does not catch rejected promises from async handlers — a + * rejection (e.g. `asUser()` throwing `AuthenticationError` before any + * response is written) escapes as an unhandledRejection, which leaves the + * request hanging and crashes Node by default. + * + * Error-handling middleware (arity 4, `(err, req, res, next)`) is returned + * unchanged: Express identifies it by function arity, so wrapping it would + * silently turn it into a regular handler. + */ +export function forwardAsyncErrors( + handler: ( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ) => unknown, +): express.RequestHandler { + if (handler.length > 3) return handler as express.RequestHandler; + return (req, res, next) => + Promise.resolve() + .then(() => handler(req, res, next)) + .catch(next); +} From 502fd533cfea98fd117973e69da22b9e8f41d136 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 11 Jun 2026 19:25:57 +0200 Subject: [PATCH 3/6] fix(appkit): return handler promises in agents routes and mask 5xx AppKitError in execute - Agents /invocations and /responses handlers now return the _handleInvoke promise so the forwardAsyncErrors wrapper applied by PluginContext.addRoute can forward rejections to the error middleware instead of letting them escape as unhandledRejection. Added an integration test locking in the returned-promise contract and a note on forwardAsyncErrors that handlers must return their promise. - Plugin.execute() now applies the same 5xx disclosure policy as the server error middleware: AppKitError messages with statusCode >= 500 are masked as "Server error" in production and preserved in development; 4xx messages are always preserved. Added unit tests covering both environments. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- packages/appkit/src/plugin/plugin.ts | 9 ++- .../appkit/src/plugin/tests/plugin.test.ts | 75 +++++++++++++++++++ packages/appkit/src/plugins/agents/agents.ts | 5 +- .../server/tests/server.integration.test.ts | 32 ++++++++ packages/appkit/src/utils/safe-handler.ts | 6 ++ 5 files changed, 122 insertions(+), 5 deletions(-) diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 17030125e..48a60fef7 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -629,16 +629,20 @@ export abstract class Plugin< } catch (error) { logger.error("Plugin execution failed", { error, plugin: this.name }); + const isDev = process.env.NODE_ENV !== "production"; + if (error instanceof AppKitError) { + // Same 5xx disclosure policy as the server error middleware: mask + // server-side error messages in production to avoid leaking internals. return { ok: false, status: error.statusCode, - message: error.message, + message: + isDev || error.statusCode < 500 ? error.message : "Server error", }; } if (hasHttpStatusCode(error)) { - const isDev = process.env.NODE_ENV !== "production"; const isClientError = error.statusCode >= 400 && error.statusCode < 500; return { ok: false, @@ -647,7 +651,6 @@ export abstract class Plugin< }; } - const isDev = process.env.NODE_ENV !== "production"; return { ok: false, status: 500, diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index 7675f57a9..49271858c 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -28,6 +28,7 @@ import { AuthenticationError, ConnectionError, ExecutionError, + ServerError, TunnelError, ValidationError, } from "../../errors"; @@ -534,6 +535,80 @@ describe("Plugin", () => { } }); + test("should redact 5xx AppKitError message 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 ServerError("internal server detail")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + // Same disclosure policy as the server error middleware: 5xx + // AppKitError messages are masked in production. + expect(result).toEqual({ + ok: false, + status: 500, + message: "Server error", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("should preserve 5xx AppKitError message 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 ServerError("internal server detail")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 500, + message: "internal server detail", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("should preserve 4xx AppKitError message 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 ValidationError("bad input")); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 400, + message: "bad input", + }); + } 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/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index c63ec094f..33e0b30df 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -771,9 +771,10 @@ export class AgentsPlugin extends Plugin implements ToolProvider { */ private mountInvokeRoutes() { if (!this.context) return; - const handler = (req: express.Request, res: express.Response) => { + // Return the promise so the forwardAsyncErrors wrapper applied by + // PluginContext.addRoute can forward rejections to the error middleware. + const handler = (req: express.Request, res: express.Response) => this._handleInvoke(req, res); - }; this.context.addRoute("post", "/invocations", handler); this.context.addRoute("post", "/responses", handler); } diff --git a/packages/appkit/src/plugins/server/tests/server.integration.test.ts b/packages/appkit/src/plugins/server/tests/server.integration.test.ts index 0553817c1..b7171fe65 100644 --- a/packages/appkit/src/plugins/server/tests/server.integration.test.ts +++ b/packages/appkit/src/plugins/server/tests/server.integration.test.ts @@ -291,6 +291,7 @@ describe("ServerPlugin error handling for rejected async handlers", () => { const suiteErrorMessages = new Set([ "boom", "buffered boom", + "returned promise boom", AuthenticationError.missingToken("user token").message, ]); const onUnhandledRejection = (reason: unknown) => { @@ -328,6 +329,18 @@ describe("ServerPlugin error handling for rejected async handlers", () => { this.context?.addRoute("get", "/buffered-error", async () => { throw new Error("buffered boom"); }); + + // Mirrors the agents plugin invoke shape: a non-async handler that + // calls an async rejecting method and RETURNS its promise. The + // forwardAsyncErrors wrapper can only catch the rejection if the + // promise is returned. + this.context?.addRoute("post", "/buffered-returned-promise", () => + this._rejectingMethod(), + ); + } + + async _rejectingMethod(): Promise { + throw new Error("returned promise boom"); } injectRoutes(router: IAppRouter) { @@ -443,6 +456,25 @@ describe("ServerPlugin error handling for rejected async handlers", () => { await new Promise((resolve) => setTimeout(resolve, 50)); expect(unhandledRejections).toEqual([]); }); + + test("buffered handler returning a rejecting method's promise returns 500 JSON without unhandled rejection", async () => { + // Locks in the contract relied on by the agents `/invocations` and + // `/responses` routes: a handler that returns its promise has rejections + // forwarded to the error middleware instead of escaping as + // unhandledRejection. + const response = await fetch(`${baseUrl}/buffered-returned-promise`, { + method: "POST", + }); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe("returned promise boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); }); describe("createApp without server plugin", () => { diff --git a/packages/appkit/src/utils/safe-handler.ts b/packages/appkit/src/utils/safe-handler.ts index 541fb50e0..e451946f3 100644 --- a/packages/appkit/src/utils/safe-handler.ts +++ b/packages/appkit/src/utils/safe-handler.ts @@ -12,6 +12,12 @@ import type express from "express"; * Error-handling middleware (arity 4, `(err, req, res, next)`) is returned * unchanged: Express identifies it by function arity, so wrapping it would * silently turn it into a regular handler. + * + * Note: handlers must RETURN their promise (e.g. `(req, res) => + * this._handle(req, res)` or `async (req, res) => { await ... }`) for + * rejections to be caught — a fire-and-forget body like + * `(req, res) => { this._handle(req, res); }` hides the promise from the + * wrapper and rejections escape as unhandledRejection again. */ export function forwardAsyncErrors( handler: ( From 235a576ec7562b0867ff22042aea8158778671e9 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Thu, 11 Jun 2026 19:45:17 +0200 Subject: [PATCH 4/6] fix(appkit): add error code to ExecutionResult so masking does not break abort detection Commit 502fd533 made Plugin.execute() mask 5xx AppKitError messages as "Server error" in production. The analytics streaming path reconstructed error semantics by string-matching the failure message ("operation was aborted", "statement was canceled", ...), so in production the mask hid the text, the abort branch never matched, and user cancellations surfaced as generic statement failures (prod-only divergence). Keep the masking (real disclosure win) and stop depending on message text instead: - Add an optional, machine-readable `code` to the failure arm of ExecutionResult. Unlike `message`, it is never masked: derived from "ABORTED" for AbortError-shaped errors, AppKitError.code, or the error class name. - Give ExecutionError.canceled() a stable EXECUTION_CANCELED code so cancellation stays distinguishable from other execution errors. - Analytics now branches on `code` first (extracted into a testable throwFromFailedSqlResult helper) and keeps message sniffing as a fallback for codeless results, so behavior is strictly no-worse. - Tests cover code survival through production masking and the cancellation -> AbortError path with masked messages. The serving plugin's startsWith("Unknown request parameters:") check was audited too: that error is thrown by resolveAndFilter() before execute(), so it never goes through the masking path and needs no change. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- packages/appkit/src/errors/execution.ts | 30 +++++++- .../appkit/src/errors/tests/errors.test.ts | 4 + .../appkit/src/plugin/execution-result.ts | 16 +++- packages/appkit/src/plugin/index.ts | 5 +- packages/appkit/src/plugin/plugin.ts | 27 ++++++- .../appkit/src/plugin/tests/plugin.test.ts | 74 ++++++++++++++++++- .../appkit/src/plugins/analytics/analytics.ts | 56 +++++++++----- .../plugins/analytics/tests/analytics.test.ts | 60 ++++++++++++++- 8 files changed, 246 insertions(+), 26 deletions(-) diff --git a/packages/appkit/src/errors/execution.ts b/packages/appkit/src/errors/execution.ts index 42de77043..e1cd7aae3 100644 --- a/packages/appkit/src/errors/execution.ts +++ b/packages/appkit/src/errors/execution.ts @@ -11,10 +11,30 @@ import { AppKitError } from "./base"; * ``` */ export class ExecutionError extends AppKitError { - readonly code = "EXECUTION_ERROR"; + /** + * Stable code carried by errors created via {@link ExecutionError.canceled}. + * Lets consumers detect cancellation programmatically without inspecting + * the message (which may be masked in production). + */ + static readonly CANCELED_CODE = "EXECUTION_CANCELED"; + + readonly code: string; readonly statusCode = 500; readonly isRetryable = false; + constructor( + message: string, + options?: { + cause?: Error; + context?: Record; + /** Stable machine-readable code (defaults to `"EXECUTION_ERROR"`). */ + code?: string; + }, + ) { + super(message, options); + this.code = options?.code ?? "EXECUTION_ERROR"; + } + /** * Create an execution error for statement failure */ @@ -26,10 +46,14 @@ export class ExecutionError extends AppKitError { } /** - * Create an execution error for canceled operation + * Create an execution error for canceled operation. + * Carries the {@link ExecutionError.CANCELED_CODE} code so cancellation + * stays distinguishable even when the message is masked in production. */ static canceled(): ExecutionError { - return new ExecutionError("Statement was canceled"); + return new ExecutionError("Statement was canceled", { + code: ExecutionError.CANCELED_CODE, + }); } /** diff --git a/packages/appkit/src/errors/tests/errors.test.ts b/packages/appkit/src/errors/tests/errors.test.ts index 347ce1c0d..6c52ce379 100644 --- a/packages/appkit/src/errors/tests/errors.test.ts +++ b/packages/appkit/src/errors/tests/errors.test.ts @@ -284,6 +284,10 @@ describe("ExecutionError", () => { test("canceled should create proper error", () => { const error = ExecutionError.canceled(); expect(error.message).toBe("Statement was canceled"); + // Stable code lets consumers detect cancellation even when the message + // is masked in production. + expect(error.code).toBe(ExecutionError.CANCELED_CODE); + expect(error.code).toBe("EXECUTION_CANCELED"); }); test("resultsClosed should create proper error", () => { diff --git a/packages/appkit/src/plugin/execution-result.ts b/packages/appkit/src/plugin/execution-result.ts index 964e89839..d2c9700b0 100644 --- a/packages/appkit/src/plugin/execution-result.ts +++ b/packages/appkit/src/plugin/execution-result.ts @@ -11,7 +11,21 @@ * 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 + * + * The optional `code` is a stable, machine-readable discriminator that is + * safe to expose to clients. Unlike `message`, it is NEVER masked in + * production, so consumers must branch on `code` (not on message text) to + * recover error semantics. It is derived from, in priority order: + * - `"ABORTED"` for errors with `name === "AbortError"` (e.g. `DOMException`) + * - `AppKitError.code` (e.g. `"EXECUTION_CANCELED"`, `"EXECUTION_ERROR"`) + * - The error's `name` for any other `Error` */ export type ExecutionResult = | { ok: true; data: T } - | { ok: false; status: number; message: string }; + | { ok: false; status: number; message: string; code?: string }; + +/** + * `code` assigned to failed {@link ExecutionResult}s caused by an aborted + * operation (any error with `name === "AbortError"`). + */ +export const ABORTED_ERROR_CODE = "ABORTED"; diff --git a/packages/appkit/src/plugin/index.ts b/packages/appkit/src/plugin/index.ts index 937652196..d54fc8a0a 100644 --- a/packages/appkit/src/plugin/index.ts +++ b/packages/appkit/src/plugin/index.ts @@ -1,4 +1,7 @@ export type { ToPlugin } from "shared"; -export type { ExecutionResult } from "./execution-result"; +export { + ABORTED_ERROR_CODE, + 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 48a60fef7..c7402aa41 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -27,7 +27,7 @@ import { import { deepMerge } from "../utils"; import { forwardAsyncErrors } from "../utils/safe-handler"; import { DevFileReader } from "./dev-reader"; -import type { ExecutionResult } from "./execution-result"; +import { ABORTED_ERROR_CODE, type ExecutionResult } from "./execution-result"; import { CacheInterceptor } from "./interceptors/cache"; import { RetryInterceptor } from "./interceptors/retry"; import { TelemetryInterceptor } from "./interceptors/telemetry"; @@ -110,6 +110,25 @@ function hasHttpStatusCode( ); } +/** + * Derive the stable, machine-readable `code` for a failed + * {@link ExecutionResult}. Unlike the message, the code is never masked in + * production, so consumers can branch on it to recover error semantics + * (e.g. distinguishing user cancellation from a statement failure). + */ +function getExecutionErrorCode(error: unknown): string | undefined { + if (error instanceof Error && error.name === "AbortError") { + return ABORTED_ERROR_CODE; + } + if (error instanceof AppKitError) { + return error.code; + } + if (error instanceof Error) { + return error.name; + } + return undefined; +} + /** * Methods that should not be proxied by asUser(). * These are lifecycle/internal methods that don't make sense @@ -630,6 +649,9 @@ export abstract class Plugin< logger.error("Plugin execution failed", { error, plugin: this.name }); const isDev = process.env.NODE_ENV !== "production"; + // Stable discriminator that survives production message masking, so + // consumers never have to string-match the (maskable) message. + const code = getExecutionErrorCode(error); if (error instanceof AppKitError) { // Same 5xx disclosure policy as the server error middleware: mask @@ -639,6 +661,7 @@ export abstract class Plugin< status: error.statusCode, message: isDev || error.statusCode < 500 ? error.message : "Server error", + code, }; } @@ -648,6 +671,7 @@ export abstract class Plugin< ok: false, status: error.statusCode, message: isDev || isClientError ? error.message : "Server error", + code, }; } @@ -656,6 +680,7 @@ export abstract class Plugin< status: 500, message: isDev && error instanceof Error ? error.message : "Server error", + code, }; } } diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index 49271858c..f4ffc1a99 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -390,6 +390,7 @@ describe("Plugin", () => { ok: false, status: 500, message: "Server error", + code: "Error", }); } finally { process.env.NODE_ENV = originalEnv; @@ -415,6 +416,7 @@ describe("Plugin", () => { ok: false, status: 500, message: "detailed debug info", + code: "Error", }); } finally { process.env.NODE_ENV = originalEnv; @@ -435,6 +437,7 @@ describe("Plugin", () => { ok: false, status: 404, message: "Not found", + code: "ApiError", }); }); @@ -452,6 +455,7 @@ describe("Plugin", () => { ok: false, status: 401, message: "Unauthorized", + code: "ApiError", }); }); @@ -469,6 +473,7 @@ describe("Plugin", () => { ok: false, status: 403, message: "Forbidden", + code: "ApiError", }); }); @@ -486,6 +491,7 @@ describe("Plugin", () => { ok: false, status: 502, message: "Bad gateway", + code: "ApiError", }); }); @@ -506,6 +512,7 @@ describe("Plugin", () => { ok: false, status: 502, message: "Server error", + code: "ApiError", }); } finally { process.env.NODE_ENV = originalEnv; @@ -529,6 +536,7 @@ describe("Plugin", () => { ok: false, status: 403, message: "Forbidden", + code: "ApiError", }); } finally { process.env.NODE_ENV = originalEnv; @@ -555,6 +563,7 @@ describe("Plugin", () => { ok: false, status: 500, message: "Server error", + code: "SERVER_ERROR", }); } finally { process.env.NODE_ENV = originalEnv; @@ -579,6 +588,7 @@ describe("Plugin", () => { ok: false, status: 500, message: "internal server detail", + code: "SERVER_ERROR", }); } finally { process.env.NODE_ENV = originalEnv; @@ -603,6 +613,7 @@ describe("Plugin", () => { ok: false, status: 400, message: "bad input", + code: "VALIDATION_ERROR", }); } finally { process.env.NODE_ENV = originalEnv; @@ -624,6 +635,7 @@ describe("Plugin", () => { ok: false, status: 401, message: expect.any(String), + code: "AUTHENTICATION_ERROR", }); }); @@ -638,7 +650,12 @@ describe("Plugin", () => { { default: {} }, false, ); - expect(result).toEqual({ ok: false, status: 400, message: "bad input" }); + expect(result).toEqual({ + ok: false, + status: 400, + message: "bad input", + code: "VALIDATION_ERROR", + }); }); test("should map ConnectionError to status 503", async () => { @@ -656,6 +673,7 @@ describe("Plugin", () => { ok: false, status: 503, message: expect.any(String), + code: "CONNECTION_ERROR", }); }); @@ -674,6 +692,7 @@ describe("Plugin", () => { ok: false, status: 502, message: "gateway failed", + code: "TUNNEL_ERROR", }); }); @@ -692,8 +711,61 @@ describe("Plugin", () => { ok: false, status: 500, message: expect.any(String), + code: "EXECUTION_ERROR", }); }); + + test("should set code ABORTED for AbortError and keep it through production masking", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new TestPlugin(config); + const abortError = new DOMException( + "The operation was aborted.", + "AbortError", + ); + const mockFn = vi.fn().mockRejectedValue(abortError); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + // The message is masked, but the machine-readable code survives so + // consumers can still detect aborts. + expect(result).toEqual({ + ok: false, + status: 500, + message: "Server error", + code: "ABORTED", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("should keep EXECUTION_CANCELED code when masking ExecutionError.canceled() 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(ExecutionError.canceled()); + + const result = await (plugin as any).execute( + mockFn, + { default: {} }, + false, + ); + expect(result).toEqual({ + ok: false, + status: 500, + message: "Server error", + code: "EXECUTION_CANCELED", + }); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); }); describe("_buildExecutionConfig", () => { diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 282e3cbc6..4e65d1579 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -24,7 +24,7 @@ import { import { assertReadOnlySql } from "../../core/agent/tools/sql-policy"; import { ExecutionError } from "../../errors"; import { createLogger } from "../../logging/logger"; -import { Plugin, toPlugin } from "../../plugin"; +import { ABORTED_ERROR_CODE, Plugin, toPlugin } from "../../plugin"; import type { PluginManifest } from "../../registry"; import { queryDefaults } from "./defaults"; import manifest from "./manifest.json"; @@ -88,6 +88,42 @@ async function* streamCallbacks( if (error) throw error; } +/** + * Re-throw a failed SQL {@link ExecutionResult} as the error the stream + * pipeline expects: an `AbortError` for user cancellations, otherwise an + * `ExecutionError.statementFailed(...)`. + * + * Branches on the machine-readable `code` first: in production, 5xx result + * messages are masked as "Server error", so message sniffing alone would + * misclassify cancellations as statement failures. Message sniffing is kept + * as a fallback for results that arrive without a code. + * + * Exported for testing. + */ +export function throwFromFailedSqlResult(result: { + message: string; + code?: string; +}): never { + const msg = result.message; + const lower = msg.toLowerCase(); + const isAborted = + result.code === ABORTED_ERROR_CODE || + result.code === ExecutionError.CANCELED_CODE || + lower.includes("operation was aborted") || + lower.includes("the request was aborted") || + lower.includes("statement was canceled"); + if (isAborted) { + throw new DOMException( + lower.includes("canceled") ? msg : "The operation was aborted.", + "AbortError", + ); + } + const inner = msg.startsWith("Statement failed: ") + ? msg.slice("Statement failed: ".length) + : msg; + throw ExecutionError.statementFailed(inner); +} + export class AnalyticsPlugin extends Plugin implements ToolProvider { /** Plugin manifest declaring metadata and resource requirements */ static manifest = manifest as PluginManifest<"analytics">; @@ -320,23 +356,7 @@ export class AnalyticsPlugin extends Plugin implements ToolProvider { ); if (!sqlResult.ok) { - const msg = sqlResult.message; - const lower = msg.toLowerCase(); - if ( - lower.includes("operation was aborted") || - lower.includes("the request was aborted") || - lower.includes("statement was canceled") - ) { - const err = new DOMException( - lower.includes("canceled") ? msg : "The operation was aborted.", - "AbortError", - ); - throw err; - } - const inner = msg.startsWith("Statement failed: ") - ? msg.slice("Statement failed: ".length) - : msg; - throw ExecutionError.statementFailed(inner); + throwFromFailedSqlResult(sqlResult); } yield sqlResult.data as AnalyticsStreamMessage; diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 08ede7bcb..5f86cc781 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -8,7 +8,11 @@ import { import { sql } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; -import { AnalyticsPlugin, analytics } from "../analytics"; +import { + AnalyticsPlugin, + analytics, + throwFromFailedSqlResult, +} from "../analytics"; import type { IAnalyticsConfig } from "../types"; // Mock CacheManager singleton with actual caching behavior @@ -749,4 +753,58 @@ describe("Analytics Plugin", () => { expect(Object.keys(entries)).toEqual(["query"]); }); }); + + describe("throwFromFailedSqlResult", () => { + test("produces AbortError from production-masked result carrying ABORTED code", () => { + // In production the 5xx message is masked as "Server error"; only the + // machine-readable code identifies the abort. + expect(() => + throwFromFailedSqlResult({ message: "Server error", code: "ABORTED" }), + ).toThrowError( + expect.objectContaining({ + name: "AbortError", + message: "The operation was aborted.", + }), + ); + }); + + test("produces AbortError from production-masked result carrying EXECUTION_CANCELED code", () => { + expect(() => + throwFromFailedSqlResult({ + message: "Server error", + code: "EXECUTION_CANCELED", + }), + ).toThrowError(expect.objectContaining({ name: "AbortError" })); + }); + + test("falls back to message sniffing when no code is present", () => { + expect(() => + throwFromFailedSqlResult({ message: "Statement was canceled" }), + ).toThrowError( + expect.objectContaining({ + name: "AbortError", + message: "Statement was canceled", + }), + ); + expect(() => + throwFromFailedSqlResult({ message: "The operation was aborted." }), + ).toThrowError(expect.objectContaining({ name: "AbortError" })); + }); + + test("throws statement failure for non-abort errors", () => { + expect(() => + throwFromFailedSqlResult({ + message: "Statement failed: syntax error", + code: "EXECUTION_ERROR", + }), + ).toThrowError("Statement failed: syntax error"); + // Masked production failure without abort semantics stays a failure. + expect(() => + throwFromFailedSqlResult({ + message: "Server error", + code: "EXECUTION_ERROR", + }), + ).toThrowError("Statement failed: Server error"); + }); + }); }); From a3020d083b0e93a1d587963e561d4506d7e3537d Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 12 Jun 2026 09:57:29 +0200 Subject: [PATCH 5/6] docs(appkit): regenerate API reference for new ExecutionResult error code exports ExecutionError and ExecutionResult gained the machine-readable error code surface (ExecutionError.CANCELED_CODE, the constructor code option, and the optional ExecutionResult.code field). Regenerate the TypeDoc API reference so the committed docs match the package code and the docs CI check passes. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- docs/docs/api/appkit/Class.ExecutionError.md | 34 ++++++++++++++----- .../api/appkit/TypeAlias.ExecutionResult.md | 9 +++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/docs/docs/api/appkit/Class.ExecutionError.md b/docs/docs/api/appkit/Class.ExecutionError.md index 75886c4dc..1ae5a6c91 100644 --- a/docs/docs/api/appkit/Class.ExecutionError.md +++ b/docs/docs/api/appkit/Class.ExecutionError.md @@ -21,24 +21,26 @@ throw new ExecutionError("Statement was canceled"); ```ts new ExecutionError(message: string, options?: { cause?: Error; + code?: string; context?: Record; }): ExecutionError; ``` #### Parameters -| Parameter | Type | -| ------ | ------ | -| `message` | `string` | -| `options?` | \{ `cause?`: `Error`; `context?`: `Record`\<`string`, `unknown`\>; \} | -| `options.cause?` | `Error` | -| `options.context?` | `Record`\<`string`, `unknown`\> | +| Parameter | Type | Description | +| ------ | ------ | ------ | +| `message` | `string` | - | +| `options?` | \{ `cause?`: `Error`; `code?`: `string`; `context?`: `Record`\<`string`, `unknown`\>; \} | - | +| `options.cause?` | `Error` | - | +| `options.code?` | `string` | Stable machine-readable code (defaults to `"EXECUTION_ERROR"`). | +| `options.context?` | `Record`\<`string`, `unknown`\> | - | #### Returns `ExecutionError` -#### Inherited from +#### Overrides [`AppKitError`](Class.AppKitError.md).[`constructor`](Class.AppKitError.md#constructor) @@ -61,7 +63,7 @@ Optional cause of the error ### code ```ts -readonly code: "EXECUTION_ERROR" = "EXECUTION_ERROR"; +readonly code: string; ``` Error code for programmatic error handling @@ -112,6 +114,18 @@ HTTP status code suggestion (can be overridden) [`AppKitError`](Class.AppKitError.md).[`statusCode`](Class.AppKitError.md#statuscode) +*** + +### CANCELED\_CODE + +```ts +readonly static CANCELED_CODE: "EXECUTION_CANCELED" = "EXECUTION_CANCELED"; +``` + +Stable code carried by errors created via [ExecutionError.canceled](#canceled). +Lets consumers detect cancellation programmatically without inspecting +the message (which may be masked in production). + ## Methods ### toJSON() @@ -157,7 +171,9 @@ Create a human-readable string representation static canceled(): ExecutionError; ``` -Create an execution error for canceled operation +Create an execution error for canceled operation. +Carries the [ExecutionError.CANCELED\_CODE](#canceled_code) code so cancellation +stays distinguishable even when the message is masked in production. #### Returns diff --git a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md index 1af9686e8..9061922fc 100644 --- a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md +++ b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md @@ -7,6 +7,7 @@ type ExecutionResult = ok: true; } | { + code?: string; message: string; ok: false; status: number; @@ -26,6 +27,14 @@ 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 +The optional `code` is a stable, machine-readable discriminator that is +safe to expose to clients. Unlike `message`, it is NEVER masked in +production, so consumers must branch on `code` (not on message text) to +recover error semantics. It is derived from, in priority order: +- `"ABORTED"` for errors with `name === "AbortError"` (e.g. `DOMException`) +- `AppKitError.code` (e.g. `"EXECUTION_CANCELED"`, `"EXECUTION_ERROR"`) +- The error's `name` for any other `Error` + ## Type Parameters | Type Parameter | From 7bbbc23fab195a8b56cc57a98cdfee06aba993f3 Mon Sep 17 00:00:00 2001 From: MarioCadenas Date: Fri, 12 Jun 2026 17:05:39 +0200 Subject: [PATCH 6/6] refactor(appkit): trim error-handling PR to async-rejection fix; drop 5xx masking + error-code The execute() 5xx-AppKitError message masking and the ExecutionResult.code machinery (plus the ExecutionError.CANCELED_CODE / ABORTED_ERROR_CODE consts and the analytics throwFromFailedSqlResult refactor that depended on them) were scope creep unrelated to the async-rejection crash fix. They have been reverted to main's existing behavior, so this change carries no new risk. Machine-readable error codes will be addressed separately as audit item A-4. The PR now contains only the reliability fix: async route-handler rejections are forwarded to a terminal Express error middleware that returns a proper HTTP error, instead of crashing the Node process. Co-authored-by: Isaac Signed-off-by: MarioCadenas --- docs/docs/api/appkit/Class.ExecutionError.md | 34 ++-- .../api/appkit/TypeAlias.ExecutionResult.md | 9 -- packages/appkit/src/errors/execution.ts | 30 +--- .../appkit/src/errors/tests/errors.test.ts | 4 - .../appkit/src/plugin/execution-result.ts | 16 +- packages/appkit/src/plugin/index.ts | 5 +- packages/appkit/src/plugin/plugin.ts | 36 +---- .../appkit/src/plugin/tests/plugin.test.ts | 149 +----------------- .../appkit/src/plugins/analytics/analytics.ts | 56 +++---- .../plugins/analytics/tests/analytics.test.ts | 60 +------ 10 files changed, 38 insertions(+), 361 deletions(-) diff --git a/docs/docs/api/appkit/Class.ExecutionError.md b/docs/docs/api/appkit/Class.ExecutionError.md index 1ae5a6c91..75886c4dc 100644 --- a/docs/docs/api/appkit/Class.ExecutionError.md +++ b/docs/docs/api/appkit/Class.ExecutionError.md @@ -21,26 +21,24 @@ throw new ExecutionError("Statement was canceled"); ```ts new ExecutionError(message: string, options?: { cause?: Error; - code?: string; context?: Record; }): ExecutionError; ``` #### Parameters -| Parameter | Type | Description | -| ------ | ------ | ------ | -| `message` | `string` | - | -| `options?` | \{ `cause?`: `Error`; `code?`: `string`; `context?`: `Record`\<`string`, `unknown`\>; \} | - | -| `options.cause?` | `Error` | - | -| `options.code?` | `string` | Stable machine-readable code (defaults to `"EXECUTION_ERROR"`). | -| `options.context?` | `Record`\<`string`, `unknown`\> | - | +| Parameter | Type | +| ------ | ------ | +| `message` | `string` | +| `options?` | \{ `cause?`: `Error`; `context?`: `Record`\<`string`, `unknown`\>; \} | +| `options.cause?` | `Error` | +| `options.context?` | `Record`\<`string`, `unknown`\> | #### Returns `ExecutionError` -#### Overrides +#### Inherited from [`AppKitError`](Class.AppKitError.md).[`constructor`](Class.AppKitError.md#constructor) @@ -63,7 +61,7 @@ Optional cause of the error ### code ```ts -readonly code: string; +readonly code: "EXECUTION_ERROR" = "EXECUTION_ERROR"; ``` Error code for programmatic error handling @@ -114,18 +112,6 @@ HTTP status code suggestion (can be overridden) [`AppKitError`](Class.AppKitError.md).[`statusCode`](Class.AppKitError.md#statuscode) -*** - -### CANCELED\_CODE - -```ts -readonly static CANCELED_CODE: "EXECUTION_CANCELED" = "EXECUTION_CANCELED"; -``` - -Stable code carried by errors created via [ExecutionError.canceled](#canceled). -Lets consumers detect cancellation programmatically without inspecting -the message (which may be masked in production). - ## Methods ### toJSON() @@ -171,9 +157,7 @@ Create a human-readable string representation static canceled(): ExecutionError; ``` -Create an execution error for canceled operation. -Carries the [ExecutionError.CANCELED\_CODE](#canceled_code) code so cancellation -stays distinguishable even when the message is masked in production. +Create an execution error for canceled operation #### Returns diff --git a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md index 9061922fc..1af9686e8 100644 --- a/docs/docs/api/appkit/TypeAlias.ExecutionResult.md +++ b/docs/docs/api/appkit/TypeAlias.ExecutionResult.md @@ -7,7 +7,6 @@ type ExecutionResult = ok: true; } | { - code?: string; message: string; ok: false; status: number; @@ -27,14 +26,6 @@ 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 -The optional `code` is a stable, machine-readable discriminator that is -safe to expose to clients. Unlike `message`, it is NEVER masked in -production, so consumers must branch on `code` (not on message text) to -recover error semantics. It is derived from, in priority order: -- `"ABORTED"` for errors with `name === "AbortError"` (e.g. `DOMException`) -- `AppKitError.code` (e.g. `"EXECUTION_CANCELED"`, `"EXECUTION_ERROR"`) -- The error's `name` for any other `Error` - ## Type Parameters | Type Parameter | diff --git a/packages/appkit/src/errors/execution.ts b/packages/appkit/src/errors/execution.ts index e1cd7aae3..42de77043 100644 --- a/packages/appkit/src/errors/execution.ts +++ b/packages/appkit/src/errors/execution.ts @@ -11,30 +11,10 @@ import { AppKitError } from "./base"; * ``` */ export class ExecutionError extends AppKitError { - /** - * Stable code carried by errors created via {@link ExecutionError.canceled}. - * Lets consumers detect cancellation programmatically without inspecting - * the message (which may be masked in production). - */ - static readonly CANCELED_CODE = "EXECUTION_CANCELED"; - - readonly code: string; + readonly code = "EXECUTION_ERROR"; readonly statusCode = 500; readonly isRetryable = false; - constructor( - message: string, - options?: { - cause?: Error; - context?: Record; - /** Stable machine-readable code (defaults to `"EXECUTION_ERROR"`). */ - code?: string; - }, - ) { - super(message, options); - this.code = options?.code ?? "EXECUTION_ERROR"; - } - /** * Create an execution error for statement failure */ @@ -46,14 +26,10 @@ export class ExecutionError extends AppKitError { } /** - * Create an execution error for canceled operation. - * Carries the {@link ExecutionError.CANCELED_CODE} code so cancellation - * stays distinguishable even when the message is masked in production. + * Create an execution error for canceled operation */ static canceled(): ExecutionError { - return new ExecutionError("Statement was canceled", { - code: ExecutionError.CANCELED_CODE, - }); + return new ExecutionError("Statement was canceled"); } /** diff --git a/packages/appkit/src/errors/tests/errors.test.ts b/packages/appkit/src/errors/tests/errors.test.ts index 6c52ce379..347ce1c0d 100644 --- a/packages/appkit/src/errors/tests/errors.test.ts +++ b/packages/appkit/src/errors/tests/errors.test.ts @@ -284,10 +284,6 @@ describe("ExecutionError", () => { test("canceled should create proper error", () => { const error = ExecutionError.canceled(); expect(error.message).toBe("Statement was canceled"); - // Stable code lets consumers detect cancellation even when the message - // is masked in production. - expect(error.code).toBe(ExecutionError.CANCELED_CODE); - expect(error.code).toBe("EXECUTION_CANCELED"); }); test("resultsClosed should create proper error", () => { diff --git a/packages/appkit/src/plugin/execution-result.ts b/packages/appkit/src/plugin/execution-result.ts index d2c9700b0..964e89839 100644 --- a/packages/appkit/src/plugin/execution-result.ts +++ b/packages/appkit/src/plugin/execution-result.ts @@ -11,21 +11,7 @@ * 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 - * - * The optional `code` is a stable, machine-readable discriminator that is - * safe to expose to clients. Unlike `message`, it is NEVER masked in - * production, so consumers must branch on `code` (not on message text) to - * recover error semantics. It is derived from, in priority order: - * - `"ABORTED"` for errors with `name === "AbortError"` (e.g. `DOMException`) - * - `AppKitError.code` (e.g. `"EXECUTION_CANCELED"`, `"EXECUTION_ERROR"`) - * - The error's `name` for any other `Error` */ export type ExecutionResult = | { ok: true; data: T } - | { ok: false; status: number; message: string; code?: string }; - -/** - * `code` assigned to failed {@link ExecutionResult}s caused by an aborted - * operation (any error with `name === "AbortError"`). - */ -export const ABORTED_ERROR_CODE = "ABORTED"; + | { ok: false; status: number; message: string }; diff --git a/packages/appkit/src/plugin/index.ts b/packages/appkit/src/plugin/index.ts index d54fc8a0a..937652196 100644 --- a/packages/appkit/src/plugin/index.ts +++ b/packages/appkit/src/plugin/index.ts @@ -1,7 +1,4 @@ export type { ToPlugin } from "shared"; -export { - ABORTED_ERROR_CODE, - type ExecutionResult, -} from "./execution-result"; +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 c7402aa41..17030125e 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -27,7 +27,7 @@ import { import { deepMerge } from "../utils"; import { forwardAsyncErrors } from "../utils/safe-handler"; import { DevFileReader } from "./dev-reader"; -import { ABORTED_ERROR_CODE, type ExecutionResult } from "./execution-result"; +import type { ExecutionResult } from "./execution-result"; import { CacheInterceptor } from "./interceptors/cache"; import { RetryInterceptor } from "./interceptors/retry"; import { TelemetryInterceptor } from "./interceptors/telemetry"; @@ -110,25 +110,6 @@ function hasHttpStatusCode( ); } -/** - * Derive the stable, machine-readable `code` for a failed - * {@link ExecutionResult}. Unlike the message, the code is never masked in - * production, so consumers can branch on it to recover error semantics - * (e.g. distinguishing user cancellation from a statement failure). - */ -function getExecutionErrorCode(error: unknown): string | undefined { - if (error instanceof Error && error.name === "AbortError") { - return ABORTED_ERROR_CODE; - } - if (error instanceof AppKitError) { - return error.code; - } - if (error instanceof Error) { - return error.name; - } - return undefined; -} - /** * Methods that should not be proxied by asUser(). * These are lifecycle/internal methods that don't make sense @@ -648,39 +629,30 @@ export abstract class Plugin< } catch (error) { logger.error("Plugin execution failed", { error, plugin: this.name }); - const isDev = process.env.NODE_ENV !== "production"; - // Stable discriminator that survives production message masking, so - // consumers never have to string-match the (maskable) message. - const code = getExecutionErrorCode(error); - if (error instanceof AppKitError) { - // Same 5xx disclosure policy as the server error middleware: mask - // server-side error messages in production to avoid leaking internals. return { ok: false, status: error.statusCode, - message: - isDev || error.statusCode < 500 ? error.message : "Server error", - code, + 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", - code, }; } + const isDev = process.env.NODE_ENV !== "production"; return { ok: false, status: 500, message: isDev && error instanceof Error ? error.message : "Server error", - code, }; } } diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index f4ffc1a99..7675f57a9 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -28,7 +28,6 @@ import { AuthenticationError, ConnectionError, ExecutionError, - ServerError, TunnelError, ValidationError, } from "../../errors"; @@ -390,7 +389,6 @@ describe("Plugin", () => { ok: false, status: 500, message: "Server error", - code: "Error", }); } finally { process.env.NODE_ENV = originalEnv; @@ -416,7 +414,6 @@ describe("Plugin", () => { ok: false, status: 500, message: "detailed debug info", - code: "Error", }); } finally { process.env.NODE_ENV = originalEnv; @@ -437,7 +434,6 @@ describe("Plugin", () => { ok: false, status: 404, message: "Not found", - code: "ApiError", }); }); @@ -455,7 +451,6 @@ describe("Plugin", () => { ok: false, status: 401, message: "Unauthorized", - code: "ApiError", }); }); @@ -473,7 +468,6 @@ describe("Plugin", () => { ok: false, status: 403, message: "Forbidden", - code: "ApiError", }); }); @@ -491,7 +485,6 @@ describe("Plugin", () => { ok: false, status: 502, message: "Bad gateway", - code: "ApiError", }); }); @@ -512,7 +505,6 @@ describe("Plugin", () => { ok: false, status: 502, message: "Server error", - code: "ApiError", }); } finally { process.env.NODE_ENV = originalEnv; @@ -536,84 +528,6 @@ describe("Plugin", () => { ok: false, status: 403, message: "Forbidden", - code: "ApiError", - }); - } finally { - process.env.NODE_ENV = originalEnv; - } - }); - - test("should redact 5xx AppKitError message 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 ServerError("internal server detail")); - - const result = await (plugin as any).execute( - mockFn, - { default: {} }, - false, - ); - // Same disclosure policy as the server error middleware: 5xx - // AppKitError messages are masked in production. - expect(result).toEqual({ - ok: false, - status: 500, - message: "Server error", - code: "SERVER_ERROR", - }); - } finally { - process.env.NODE_ENV = originalEnv; - } - }); - - test("should preserve 5xx AppKitError message 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 ServerError("internal server detail")); - - const result = await (plugin as any).execute( - mockFn, - { default: {} }, - false, - ); - expect(result).toEqual({ - ok: false, - status: 500, - message: "internal server detail", - code: "SERVER_ERROR", - }); - } finally { - process.env.NODE_ENV = originalEnv; - } - }); - - test("should preserve 4xx AppKitError message 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 ValidationError("bad input")); - - const result = await (plugin as any).execute( - mockFn, - { default: {} }, - false, - ); - expect(result).toEqual({ - ok: false, - status: 400, - message: "bad input", - code: "VALIDATION_ERROR", }); } finally { process.env.NODE_ENV = originalEnv; @@ -635,7 +549,6 @@ describe("Plugin", () => { ok: false, status: 401, message: expect.any(String), - code: "AUTHENTICATION_ERROR", }); }); @@ -650,12 +563,7 @@ describe("Plugin", () => { { default: {} }, false, ); - expect(result).toEqual({ - ok: false, - status: 400, - message: "bad input", - code: "VALIDATION_ERROR", - }); + expect(result).toEqual({ ok: false, status: 400, message: "bad input" }); }); test("should map ConnectionError to status 503", async () => { @@ -673,7 +581,6 @@ describe("Plugin", () => { ok: false, status: 503, message: expect.any(String), - code: "CONNECTION_ERROR", }); }); @@ -692,7 +599,6 @@ describe("Plugin", () => { ok: false, status: 502, message: "gateway failed", - code: "TUNNEL_ERROR", }); }); @@ -711,61 +617,8 @@ describe("Plugin", () => { ok: false, status: 500, message: expect.any(String), - code: "EXECUTION_ERROR", }); }); - - test("should set code ABORTED for AbortError and keep it through production masking", async () => { - const originalEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "production"; - try { - const plugin = new TestPlugin(config); - const abortError = new DOMException( - "The operation was aborted.", - "AbortError", - ); - const mockFn = vi.fn().mockRejectedValue(abortError); - - const result = await (plugin as any).execute( - mockFn, - { default: {} }, - false, - ); - // The message is masked, but the machine-readable code survives so - // consumers can still detect aborts. - expect(result).toEqual({ - ok: false, - status: 500, - message: "Server error", - code: "ABORTED", - }); - } finally { - process.env.NODE_ENV = originalEnv; - } - }); - - test("should keep EXECUTION_CANCELED code when masking ExecutionError.canceled() 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(ExecutionError.canceled()); - - const result = await (plugin as any).execute( - mockFn, - { default: {} }, - false, - ); - expect(result).toEqual({ - ok: false, - status: 500, - message: "Server error", - code: "EXECUTION_CANCELED", - }); - } finally { - process.env.NODE_ENV = originalEnv; - } - }); }); describe("_buildExecutionConfig", () => { diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 4e65d1579..282e3cbc6 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -24,7 +24,7 @@ import { import { assertReadOnlySql } from "../../core/agent/tools/sql-policy"; import { ExecutionError } from "../../errors"; import { createLogger } from "../../logging/logger"; -import { ABORTED_ERROR_CODE, Plugin, toPlugin } from "../../plugin"; +import { Plugin, toPlugin } from "../../plugin"; import type { PluginManifest } from "../../registry"; import { queryDefaults } from "./defaults"; import manifest from "./manifest.json"; @@ -88,42 +88,6 @@ async function* streamCallbacks( if (error) throw error; } -/** - * Re-throw a failed SQL {@link ExecutionResult} as the error the stream - * pipeline expects: an `AbortError` for user cancellations, otherwise an - * `ExecutionError.statementFailed(...)`. - * - * Branches on the machine-readable `code` first: in production, 5xx result - * messages are masked as "Server error", so message sniffing alone would - * misclassify cancellations as statement failures. Message sniffing is kept - * as a fallback for results that arrive without a code. - * - * Exported for testing. - */ -export function throwFromFailedSqlResult(result: { - message: string; - code?: string; -}): never { - const msg = result.message; - const lower = msg.toLowerCase(); - const isAborted = - result.code === ABORTED_ERROR_CODE || - result.code === ExecutionError.CANCELED_CODE || - lower.includes("operation was aborted") || - lower.includes("the request was aborted") || - lower.includes("statement was canceled"); - if (isAborted) { - throw new DOMException( - lower.includes("canceled") ? msg : "The operation was aborted.", - "AbortError", - ); - } - const inner = msg.startsWith("Statement failed: ") - ? msg.slice("Statement failed: ".length) - : msg; - throw ExecutionError.statementFailed(inner); -} - export class AnalyticsPlugin extends Plugin implements ToolProvider { /** Plugin manifest declaring metadata and resource requirements */ static manifest = manifest as PluginManifest<"analytics">; @@ -356,7 +320,23 @@ export class AnalyticsPlugin extends Plugin implements ToolProvider { ); if (!sqlResult.ok) { - throwFromFailedSqlResult(sqlResult); + const msg = sqlResult.message; + const lower = msg.toLowerCase(); + if ( + lower.includes("operation was aborted") || + lower.includes("the request was aborted") || + lower.includes("statement was canceled") + ) { + const err = new DOMException( + lower.includes("canceled") ? msg : "The operation was aborted.", + "AbortError", + ); + throw err; + } + const inner = msg.startsWith("Statement failed: ") + ? msg.slice("Statement failed: ".length) + : msg; + throw ExecutionError.statementFailed(inner); } yield sqlResult.data as AnalyticsStreamMessage; diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 5f86cc781..08ede7bcb 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -8,11 +8,7 @@ import { import { sql } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; -import { - AnalyticsPlugin, - analytics, - throwFromFailedSqlResult, -} from "../analytics"; +import { AnalyticsPlugin, analytics } from "../analytics"; import type { IAnalyticsConfig } from "../types"; // Mock CacheManager singleton with actual caching behavior @@ -753,58 +749,4 @@ describe("Analytics Plugin", () => { expect(Object.keys(entries)).toEqual(["query"]); }); }); - - describe("throwFromFailedSqlResult", () => { - test("produces AbortError from production-masked result carrying ABORTED code", () => { - // In production the 5xx message is masked as "Server error"; only the - // machine-readable code identifies the abort. - expect(() => - throwFromFailedSqlResult({ message: "Server error", code: "ABORTED" }), - ).toThrowError( - expect.objectContaining({ - name: "AbortError", - message: "The operation was aborted.", - }), - ); - }); - - test("produces AbortError from production-masked result carrying EXECUTION_CANCELED code", () => { - expect(() => - throwFromFailedSqlResult({ - message: "Server error", - code: "EXECUTION_CANCELED", - }), - ).toThrowError(expect.objectContaining({ name: "AbortError" })); - }); - - test("falls back to message sniffing when no code is present", () => { - expect(() => - throwFromFailedSqlResult({ message: "Statement was canceled" }), - ).toThrowError( - expect.objectContaining({ - name: "AbortError", - message: "Statement was canceled", - }), - ); - expect(() => - throwFromFailedSqlResult({ message: "The operation was aborted." }), - ).toThrowError(expect.objectContaining({ name: "AbortError" })); - }); - - test("throws statement failure for non-abort errors", () => { - expect(() => - throwFromFailedSqlResult({ - message: "Statement failed: syntax error", - code: "EXECUTION_ERROR", - }), - ).toThrowError("Statement failed: syntax error"); - // Masked production failure without abort semantics stays a failure. - expect(() => - throwFromFailedSqlResult({ - message: "Server error", - code: "EXECUTION_ERROR", - }), - ).toThrowError("Statement failed: Server error"); - }); - }); });