fix(FN-1287): standardize dashboard API error responses
- Add shared ApiError class and response helpers for consistent error shape - Migrate routes and mission-routes handlers to use standardized API errors - Update server error middleware, rate limiter, and node metrics error handling to use shared helpers - Expand dashboard tests with ApiError coverage and standardized response assertions - Add a changeset documenting the @gsxdsm/fusion patch for API error shape standardization
This commit is contained in:
@@ -313,6 +313,15 @@ describe("Node routes", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("GET /api/nodes/:id/metrics returns 501 for remote nodes", async () => {
|
||||
mockGetNode.mockResolvedValue(makeNode({ id: "node_2", type: "remote" }));
|
||||
|
||||
const res = await request(app, "GET", "/api/nodes/node_2/metrics");
|
||||
|
||||
expect(res.status).toBe(501);
|
||||
expect(res.body).toEqual({ error: "Remote node metrics not yet implemented" });
|
||||
});
|
||||
|
||||
it("PATCH /api/projects/:id assigns project to node when nodeId is provided", async () => {
|
||||
const res = await request(
|
||||
app,
|
||||
|
||||
239
packages/dashboard/src/api-error.test.ts
Normal file
239
packages/dashboard/src/api-error.test.ts
Normal file
@@ -0,0 +1,239 @@
|
||||
// @vitest-environment node
|
||||
|
||||
import type { NextFunction, Request, Response } from "express";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
ApiError,
|
||||
badRequest,
|
||||
catchHandler,
|
||||
conflict,
|
||||
internalError,
|
||||
notFound,
|
||||
rateLimited,
|
||||
sendErrorResponse,
|
||||
unauthorized,
|
||||
} from "./api-error.js";
|
||||
|
||||
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined);
|
||||
|
||||
interface MockResponse {
|
||||
res: Response;
|
||||
statusMock: ReturnType<typeof vi.fn>;
|
||||
jsonMock: ReturnType<typeof vi.fn>;
|
||||
}
|
||||
|
||||
function createMockResponse(overrides?: Partial<Response>, requestOverrides?: Partial<Request>): MockResponse {
|
||||
const statusMock = vi.fn();
|
||||
const jsonMock = vi.fn();
|
||||
statusMock.mockReturnValue({ json: jsonMock });
|
||||
|
||||
const req = {
|
||||
method: "GET",
|
||||
path: "/api/test",
|
||||
originalUrl: "/api/test?x=1",
|
||||
...requestOverrides,
|
||||
} as Request;
|
||||
|
||||
const res = {
|
||||
req,
|
||||
headersSent: false,
|
||||
status: statusMock,
|
||||
json: jsonMock,
|
||||
...overrides,
|
||||
} as unknown as Response;
|
||||
|
||||
return {
|
||||
res,
|
||||
statusMock,
|
||||
jsonMock,
|
||||
};
|
||||
}
|
||||
|
||||
describe("ApiError", () => {
|
||||
it("sets statusCode, message, and details", () => {
|
||||
const details = { foo: "bar" };
|
||||
const error = new ApiError(418, "teapot", details);
|
||||
|
||||
expect(error.statusCode).toBe(418);
|
||||
expect(error.message).toBe("teapot");
|
||||
expect(error.details).toEqual(details);
|
||||
expect(error.name).toBe("ApiError");
|
||||
});
|
||||
|
||||
it("defaults isOperational to true", () => {
|
||||
const error = new ApiError(400, "bad request");
|
||||
expect(error.isOperational).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("sendErrorResponse", () => {
|
||||
beforeEach(() => {
|
||||
consoleErrorSpy.mockClear();
|
||||
});
|
||||
|
||||
it("sends standard { error: string } payload", () => {
|
||||
const { res, statusMock, jsonMock } = createMockResponse();
|
||||
|
||||
sendErrorResponse(res, 400, "Bad request");
|
||||
|
||||
expect(statusMock).toHaveBeenCalledWith(400);
|
||||
expect(jsonMock).toHaveBeenCalledWith({ error: "Bad request" });
|
||||
});
|
||||
|
||||
it("includes details when provided", () => {
|
||||
const { res, jsonMock } = createMockResponse();
|
||||
const details = { projectCount: 2, globalCount: 10 };
|
||||
|
||||
sendErrorResponse(res, 500, "Import failed", { details });
|
||||
|
||||
expect(jsonMock).toHaveBeenCalledWith({ error: "Import failed", details });
|
||||
});
|
||||
|
||||
it("omits details when not provided", () => {
|
||||
const { res, jsonMock } = createMockResponse();
|
||||
|
||||
sendErrorResponse(res, 500, "Server exploded");
|
||||
|
||||
expect(jsonMock).toHaveBeenCalledWith({ error: "Server exploded" });
|
||||
});
|
||||
|
||||
it("logs 5xx errors with structured metadata", () => {
|
||||
const { res } = createMockResponse();
|
||||
|
||||
sendErrorResponse(res, 500, "Internal issue");
|
||||
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith("[api:error]", {
|
||||
method: "GET",
|
||||
path: "/api/test?x=1",
|
||||
statusCode: 500,
|
||||
message: "Internal issue",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not log 4xx errors", () => {
|
||||
const { res } = createMockResponse();
|
||||
|
||||
sendErrorResponse(res, 404, "Not found");
|
||||
|
||||
expect(consoleErrorSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("catchHandler", () => {
|
||||
beforeEach(() => {
|
||||
consoleErrorSpy.mockClear();
|
||||
});
|
||||
|
||||
it("catches ApiError and sends status/message/details", async () => {
|
||||
const details = { field: "name" };
|
||||
const handler = catchHandler(async () => {
|
||||
throw badRequest("Invalid input", details);
|
||||
});
|
||||
const { res, statusMock, jsonMock } = createMockResponse();
|
||||
const next = vi.fn<NextFunction>();
|
||||
|
||||
await handler({} as Request, res, next);
|
||||
|
||||
expect(statusMock).toHaveBeenCalledWith(400);
|
||||
expect(jsonMock).toHaveBeenCalledWith({ error: "Invalid input", details });
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
expect(consoleErrorSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("catches generic Error and sends 500 with error message", async () => {
|
||||
const handler = catchHandler(async () => {
|
||||
throw new Error("boom");
|
||||
});
|
||||
const { res, statusMock, jsonMock } = createMockResponse();
|
||||
const next = vi.fn<NextFunction>();
|
||||
|
||||
await handler({} as Request, res, next);
|
||||
|
||||
expect(statusMock).toHaveBeenCalledWith(500);
|
||||
expect(jsonMock).toHaveBeenCalledWith({ error: "boom" });
|
||||
expect(consoleErrorSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("calls next(err) when headers are already sent", async () => {
|
||||
const thrown = new Error("already sent");
|
||||
const handler = catchHandler(async () => {
|
||||
throw thrown;
|
||||
});
|
||||
const { res, statusMock, jsonMock } = createMockResponse({ headersSent: true });
|
||||
const next = vi.fn<NextFunction>();
|
||||
|
||||
await handler({} as Request, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalledWith(thrown);
|
||||
expect(statusMock).not.toHaveBeenCalled();
|
||||
expect(jsonMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows successful handlers to continue without interception", async () => {
|
||||
const handler = catchHandler(async (_req, _res, next) => {
|
||||
next();
|
||||
});
|
||||
const { res, statusMock, jsonMock } = createMockResponse();
|
||||
const next = vi.fn<NextFunction>();
|
||||
|
||||
await handler({} as Request, res, next);
|
||||
|
||||
expect(next).toHaveBeenCalledTimes(1);
|
||||
expect(statusMock).not.toHaveBeenCalled();
|
||||
expect(jsonMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("error factories", () => {
|
||||
it("badRequest creates ApiError(400)", () => {
|
||||
const error = badRequest("msg");
|
||||
expect(error).toBeInstanceOf(ApiError);
|
||||
expect(error.statusCode).toBe(400);
|
||||
expect(error.message).toBe("msg");
|
||||
expect(error.details).toBeUndefined();
|
||||
});
|
||||
|
||||
it("badRequest supports details", () => {
|
||||
const error = badRequest("msg", { field: "x" });
|
||||
expect(error.statusCode).toBe(400);
|
||||
expect(error.details).toEqual({ field: "x" });
|
||||
});
|
||||
|
||||
it("unauthorized creates ApiError(401)", () => {
|
||||
const error = unauthorized("msg");
|
||||
expect(error.statusCode).toBe(401);
|
||||
expect(error.message).toBe("msg");
|
||||
});
|
||||
|
||||
it("notFound creates ApiError(404)", () => {
|
||||
const error = notFound("msg");
|
||||
expect(error.statusCode).toBe(404);
|
||||
expect(error.message).toBe("msg");
|
||||
});
|
||||
|
||||
it("conflict creates ApiError(409)", () => {
|
||||
const error = conflict("msg");
|
||||
expect(error.statusCode).toBe(409);
|
||||
expect(error.message).toBe("msg");
|
||||
});
|
||||
|
||||
it("rateLimited creates ApiError(429) with undefined retryAfter by default", () => {
|
||||
const error = rateLimited("msg");
|
||||
expect(error.statusCode).toBe(429);
|
||||
expect(error.message).toBe("msg");
|
||||
expect(error.details).toEqual({ retryAfter: undefined });
|
||||
});
|
||||
|
||||
it("rateLimited creates ApiError(429) with retryAfter details when provided", () => {
|
||||
const error = rateLimited("msg", 60);
|
||||
expect(error.statusCode).toBe(429);
|
||||
expect(error.message).toBe("msg");
|
||||
expect(error.details).toEqual({ retryAfter: 60 });
|
||||
});
|
||||
|
||||
it("internalError creates ApiError(500)", () => {
|
||||
const error = internalError("msg");
|
||||
expect(error.statusCode).toBe(500);
|
||||
expect(error.message).toBe("msg");
|
||||
});
|
||||
});
|
||||
99
packages/dashboard/src/api-error.ts
Normal file
99
packages/dashboard/src/api-error.ts
Normal file
@@ -0,0 +1,99 @@
|
||||
import type { NextFunction, Request, RequestHandler, Response } from "express";
|
||||
|
||||
export interface ApiErrorResponse {
|
||||
error: string;
|
||||
details?: Record<string, unknown>;
|
||||
}
|
||||
|
||||
export interface SendErrorOptions {
|
||||
details?: Record<string, unknown>;
|
||||
}
|
||||
|
||||
export class ApiError extends Error {
|
||||
public readonly statusCode: number;
|
||||
public readonly details?: Record<string, unknown>;
|
||||
public readonly isOperational: boolean;
|
||||
|
||||
constructor(statusCode: number, message: string, details?: Record<string, unknown>) {
|
||||
super(message);
|
||||
this.name = "ApiError";
|
||||
this.statusCode = statusCode;
|
||||
this.details = details;
|
||||
this.isOperational = true;
|
||||
}
|
||||
}
|
||||
|
||||
export function sendErrorResponse(
|
||||
res: Response,
|
||||
statusCode: number,
|
||||
message: string,
|
||||
options?: SendErrorOptions,
|
||||
): Response<ApiErrorResponse> {
|
||||
if (statusCode >= 500) {
|
||||
const request = res.req;
|
||||
console.error("[api:error]", {
|
||||
method: request?.method,
|
||||
path: request?.originalUrl ?? request?.path,
|
||||
statusCode,
|
||||
message,
|
||||
});
|
||||
}
|
||||
|
||||
const payload: ApiErrorResponse = { error: message };
|
||||
if (options?.details !== undefined) {
|
||||
payload.details = options.details;
|
||||
}
|
||||
|
||||
return res.status(statusCode).json(payload);
|
||||
}
|
||||
|
||||
type AsyncHandler = (req: Request, res: Response, next: NextFunction) => Promise<unknown> | unknown;
|
||||
|
||||
export function catchHandler(fn: AsyncHandler): RequestHandler {
|
||||
return async (req, res, next) => {
|
||||
try {
|
||||
await fn(req, res, next);
|
||||
} catch (error) {
|
||||
if (res.headersSent) {
|
||||
next(error);
|
||||
return;
|
||||
}
|
||||
|
||||
if (error instanceof ApiError) {
|
||||
sendErrorResponse(res, error.statusCode, error.message, { details: error.details });
|
||||
return;
|
||||
}
|
||||
|
||||
if (error instanceof Error) {
|
||||
sendErrorResponse(res, 500, error.message);
|
||||
return;
|
||||
}
|
||||
|
||||
sendErrorResponse(res, 500, "Internal server error");
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
export function badRequest(message: string, details?: Record<string, unknown>): ApiError {
|
||||
return new ApiError(400, message, details);
|
||||
}
|
||||
|
||||
export function unauthorized(message: string): ApiError {
|
||||
return new ApiError(401, message);
|
||||
}
|
||||
|
||||
export function notFound(message: string): ApiError {
|
||||
return new ApiError(404, message);
|
||||
}
|
||||
|
||||
export function conflict(message: string): ApiError {
|
||||
return new ApiError(409, message);
|
||||
}
|
||||
|
||||
export function rateLimited(message: string, retryAfter?: number): ApiError {
|
||||
return new ApiError(429, message, { retryAfter });
|
||||
}
|
||||
|
||||
export function internalError(message: string): ApiError {
|
||||
return new ApiError(500, message);
|
||||
}
|
||||
@@ -2,6 +2,19 @@ export { createServer, type ServerOptions } from "./server.js";
|
||||
export { GitHubClient, isPrMergeReady, type PrMergeStatus, type PrCheckStatus, type ReviewDecision, type MergePrParams, type FindPrParams } from "./github.js";
|
||||
export { rateLimit, RATE_LIMITS, type RateLimitOptions } from "./rate-limit.js";
|
||||
export { GitHubPollingService, type GitHubPollingServiceOptions, type TaskWatchInput, type WatchedBadgeType } from "./github-poll.js";
|
||||
export {
|
||||
ApiError,
|
||||
type ApiErrorResponse,
|
||||
type SendErrorOptions,
|
||||
sendErrorResponse,
|
||||
catchHandler,
|
||||
badRequest,
|
||||
unauthorized,
|
||||
notFound,
|
||||
conflict,
|
||||
rateLimited,
|
||||
internalError,
|
||||
} from "./api-error.js";
|
||||
export {
|
||||
type BadgePubSub,
|
||||
type BadgePubSubEvents,
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -1,4 +1,5 @@
|
||||
import type { Request, Response, NextFunction } from "express";
|
||||
import { sendErrorResponse } from "./api-error.js";
|
||||
|
||||
export interface RateLimitOptions {
|
||||
/** Time window in milliseconds (default: 60000 = 1 minute) */
|
||||
@@ -66,7 +67,7 @@ export function rateLimit(options: RateLimitOptions = {}) {
|
||||
|
||||
if (record.count > max) {
|
||||
res.setHeader("Retry-After", String(resetSeconds));
|
||||
res.status(429).json({ error: message });
|
||||
sendErrorResponse(res, 429, message);
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -178,6 +178,59 @@ describe("GET /tasks", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("Standardized error responses", () => {
|
||||
let store: TaskStore;
|
||||
|
||||
beforeEach(() => {
|
||||
store = createMockStore();
|
||||
});
|
||||
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", createApiRoutes(store));
|
||||
return app;
|
||||
}
|
||||
|
||||
it("returns 400 validation errors as { error } without success field", async () => {
|
||||
const res = await GET(buildApp(), "/api/tasks?limit=-1");
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body).toEqual({ error: expect.stringContaining("limit") });
|
||||
expect(res.body).not.toHaveProperty("success");
|
||||
});
|
||||
|
||||
it("returns 404 not-found errors as { error }", async () => {
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockRejectedValueOnce(Object.assign(new Error("Task NOPE not found"), { code: "ENOENT" }));
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/NOPE");
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body).toEqual({ error: expect.stringContaining("not found") });
|
||||
});
|
||||
|
||||
it("returns 500 errors as { error } and logs to console.error", async () => {
|
||||
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined);
|
||||
(store.getSettings as ReturnType<typeof vi.fn>).mockRejectedValueOnce(new Error("Config read failed"));
|
||||
|
||||
const res = await GET(buildApp(), "/api/settings");
|
||||
|
||||
expect(res.status).toBe(500);
|
||||
expect(res.body).toEqual({ error: "Config read failed" });
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
"[api:error]",
|
||||
expect.objectContaining({
|
||||
method: "GET",
|
||||
path: "/api/settings",
|
||||
statusCode: 500,
|
||||
message: "Config read failed",
|
||||
}),
|
||||
);
|
||||
|
||||
consoleErrorSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /projects", () => {
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
@@ -3555,7 +3608,7 @@ describe("Pause/Unpause endpoints", () => {
|
||||
|
||||
expect(res.status).toBe(429);
|
||||
expect(res.body.error).toContain("rate limit exceeded");
|
||||
expect(res.body.resetAt).toBe("2026-03-30T12:05:00.000Z");
|
||||
expect(res.body.details?.resetAt).toBe("2026-03-30T12:05:00.000Z");
|
||||
|
||||
canMakeRequestSpy.mockRestore();
|
||||
getResetTimeSpy.mockRestore();
|
||||
@@ -3860,7 +3913,7 @@ describe("POST /github/issues/import", () => {
|
||||
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.error).toContain("already imported");
|
||||
expect(res.body.existingTaskId).toBe("FN-002");
|
||||
expect(res.body.details?.existingTaskId).toBe("FN-002");
|
||||
expect(store.createTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -6745,7 +6798,7 @@ describe("Terminal session routes", () => {
|
||||
expect(res.status).toBe(503);
|
||||
expect(res.body).toEqual({
|
||||
error: "Maximum terminal sessions reached. Please close an existing terminal and try again.",
|
||||
code: "max_sessions",
|
||||
details: { code: "max_sessions" },
|
||||
});
|
||||
|
||||
vi.restoreAllMocks();
|
||||
@@ -6774,7 +6827,7 @@ describe("Terminal session routes", () => {
|
||||
);
|
||||
|
||||
expect(res.status).toBe(status);
|
||||
expect(res.body).toEqual({ error, code });
|
||||
expect(res.body).toEqual({ error, details: { code } });
|
||||
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
@@ -8854,7 +8907,7 @@ describe("POST /api/agents/:id/runs", () => {
|
||||
const res2 = await REQUEST(buildApp(), "POST", `/api/agents/${agentId}/runs`);
|
||||
expect(res2.status).toBe(409);
|
||||
expect(res2.body.error).toContain("active run");
|
||||
expect(res2.body.runId).toBeTruthy();
|
||||
expect(res2.body.details?.runId).toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -8,6 +8,7 @@ import type { AuthStorageLike, ModelRegistryLike } from "./routes.js";
|
||||
import { createApiRoutes } from "./routes.js";
|
||||
import { createSSE } from "./sse.js";
|
||||
import { rateLimit, RATE_LIMITS } from "./rate-limit.js";
|
||||
import { ApiError, sendErrorResponse } from "./api-error.js";
|
||||
import { getOrCreateProjectStore, evictAllProjectStores } from "./project-store-resolver.js";
|
||||
import { getTerminalService, type TerminalSession, STALE_SESSION_THRESHOLD_MS } from "./terminal-service.js";
|
||||
import { WebSocketServer, type WebSocket } from "ws";
|
||||
@@ -179,7 +180,7 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
|
||||
const scopedStore = await getOrCreateProjectStore(projectId);
|
||||
createSSE(scopedStore, scopedStore.getMissionStore(), aiSessionStore)(req, res);
|
||||
} catch (err: any) {
|
||||
res.status(500).json({ error: err.message ?? "Failed to open project event stream" });
|
||||
sendErrorResponse(res, 500, err.message ?? "Failed to open project event stream");
|
||||
}
|
||||
});
|
||||
|
||||
@@ -394,21 +395,31 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
|
||||
|
||||
// API 404 Handler - Return JSON for unmatched API routes (instead of falling through to SPA)
|
||||
app.use("/api", (_req: express.Request, res: express.Response) => {
|
||||
res.status(404).json({ error: "Not found" });
|
||||
sendErrorResponse(res, 404, "Not found");
|
||||
});
|
||||
|
||||
// API Error Handling Middleware - MUST be after API routes but before SPA fallback
|
||||
// This ensures API errors return JSON instead of falling through to the SPA fallback (which returns HTML)
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
app.use("/api", (err: Error, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
|
||||
console.error("[api:error]", err);
|
||||
|
||||
// Ensure we send a JSON response even if headers already sent (though this is a edge case)
|
||||
app.use("/api", (err: unknown, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
|
||||
if (res.headersSent) {
|
||||
return;
|
||||
}
|
||||
|
||||
res.status(500).json({ error: "Internal server error" });
|
||||
|
||||
if (err instanceof ApiError) {
|
||||
sendErrorResponse(res, err.statusCode, err.message, { details: err.details });
|
||||
return;
|
||||
}
|
||||
|
||||
const fallbackMessage = "Internal server error";
|
||||
const message =
|
||||
process.env.NODE_ENV === "production"
|
||||
? fallbackMessage
|
||||
: err instanceof Error && err.message
|
||||
? err.message
|
||||
: fallbackMessage;
|
||||
|
||||
sendErrorResponse(res, 500, message);
|
||||
});
|
||||
|
||||
if (!isHeadless) {
|
||||
|
||||
Reference in New Issue
Block a user