Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/appkit/src/core/plugin-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -287,7 +288,7 @@ export class PluginContext {
if (typeof app[method] === "function") {
(app[method] as (...a: unknown[]) => void)(
route.path,
...route.handlers,
...route.handlers.map(forwardAsyncErrors),
);
}
});
Expand All @@ -299,7 +300,7 @@ export class PluginContext {
): void {
if (!this.routeTarget) return;
this.routeTarget.addExtension((app) => {
app.use(path, ...handlers);
app.use(path, ...handlers.map(forwardAsyncErrors));
});
}
}
Expand Down
76 changes: 68 additions & 8 deletions packages/appkit/src/core/tests/plugin-context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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 });

Expand All @@ -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();

Expand All @@ -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);

Expand All @@ -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", () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/appkit/src/plugin/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -666,7 +667,7 @@ export abstract class Plugin<
): void {
const { name, method, path, handler } = config;

router[method](path, handler);
router[method](path, forwardAsyncErrors(handler));

const fullPath = `/api/${this.name}${path}`;
this.registerEndpoint(name, fullPath);
Expand Down
5 changes: 3 additions & 2 deletions packages/appkit/src/plugins/agents/agents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -776,9 +776,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);
}
Expand Down
88 changes: 87 additions & 1 deletion packages/appkit/src/plugins/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -192,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.
*/
Expand Down Expand Up @@ -505,6 +516,81 @@ 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.statusCode;
return (
typeof statusCode === "number" &&
Number.isInteger(statusCode) &&
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()`:
* - 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(
err: unknown,
req: express.Request,
res: express.Response,
next: express.NextFunction,
) {
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;
}

const isDev = process.env.NODE_ENV !== "production";
const message = err instanceof Error ? err.message : "Server error";

res.status(statusCode).json({
error: isDev || isClientError ? message : "Server error",
});
}

/**
* @internal
*/
Expand Down
Loading
Loading