From 43eeee2e0471faa59df08bc8e5efe72a999d7e76 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Sun, 6 Sep 2026 01:03:40 +0200 Subject: [PATCH] fix(tools): address review findings on error codes, registerTool assertions, and strict typing --- src/client/api-client.ts | 14 ++++----- src/tools/common.ts | 18 +++++++---- tests/tools.test.ts | 68 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 82 insertions(+), 18 deletions(-) diff --git a/src/client/api-client.ts b/src/client/api-client.ts index 1e9191a..74bf363 100644 --- a/src/client/api-client.ts +++ b/src/client/api-client.ts @@ -48,7 +48,7 @@ export class OpenProjectAuthenticationError extends OpenProjectError { message = "Authentication failed. Please check your OpenProject API key.", options?: { errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 401, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_AUTH_ERROR", ...options }); + super(message, { statusCode: 401, code: options?.code ?? "OPENPROJECT_AUTH_ERROR", ...options }); this.name = "OpenProjectAuthenticationError"; } } @@ -58,7 +58,7 @@ export class OpenProjectForbiddenError extends OpenProjectError { message = "Access forbidden. You do not have permission to access this resource.", options?: { errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 403, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_FORBIDDEN", ...options }); + super(message, { statusCode: 403, code: options?.code ?? "OPENPROJECT_FORBIDDEN", ...options }); this.name = "OpenProjectForbiddenError"; } } @@ -68,7 +68,7 @@ export class OpenProjectNotFoundError extends OpenProjectError { message = "Resource not found.", options?: { errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 404, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_NOT_FOUND", ...options }); + super(message, { statusCode: 404, code: options?.code ?? "OPENPROJECT_NOT_FOUND", ...options }); this.name = "OpenProjectNotFoundError"; } } @@ -78,7 +78,7 @@ export class OpenProjectConflictError extends OpenProjectError { message = "Conflict updating resource. Lock version mismatch.", options?: { errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 409, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_CONFLICT", ...options }); + super(message, { statusCode: 409, code: options?.code ?? "OPENPROJECT_CONFLICT", ...options }); this.name = "OpenProjectConflictError"; } } @@ -88,7 +88,7 @@ export class OpenProjectValidationError extends OpenProjectError { message = "Validation failed for request data.", options?: { errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 422, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_VALIDATION_ERROR", ...options }); + super(message, { statusCode: 422, code: options?.code ?? "OPENPROJECT_VALIDATION_ERROR", ...options }); this.name = "OpenProjectValidationError"; } } @@ -100,7 +100,7 @@ export class OpenProjectRateLimitError extends OpenProjectError { message = "Rate limit exceeded. Please wait before retrying.", options?: { retryAfter?: number; errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: 429, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_RATE_LIMIT", ...options }); + super(message, { statusCode: 429, code: options?.code ?? "OPENPROJECT_RATE_LIMIT", ...options }); this.name = "OpenProjectRateLimitError"; this.retryAfter = options?.retryAfter; } @@ -111,7 +111,7 @@ export class OpenProjectServerError extends OpenProjectError { message = "OpenProject internal server error.", options?: { statusCode?: number; errorIdentifier?: string; details?: unknown; cause?: unknown; code?: string } ) { - super(message, { statusCode: options?.statusCode ?? 500, code: options?.code ?? options?.errorIdentifier ?? "OPENPROJECT_SERVER_ERROR", ...options }); + super(message, { statusCode: options?.statusCode ?? 500, code: options?.code ?? "OPENPROJECT_SERVER_ERROR", ...options }); this.name = "OpenProjectServerError"; } } diff --git a/src/tools/common.ts b/src/tools/common.ts index 123d33b..b4a72a4 100644 --- a/src/tools/common.ts +++ b/src/tools/common.ts @@ -53,25 +53,31 @@ export function formatToolError(error: unknown): McpToolResponse { /** * Definition interface for OpenProject MCP tools. */ -export interface ToolDefinition { +export interface ToolDefinition< + TShape extends ZodRawShape = ZodRawShape, + TArgs = Record, +> { name: string; description: string; parameters?: TShape; readOnly: boolean; - execute: (args: any) => Promise; + execute: (args: TArgs) => Promise; } /** * Registers a ToolDefinition instance on an McpServer instance. */ -export function registerTool(server: McpServer, tool: ToolDefinition): void { +export function registerTool< + TShape extends ZodRawShape = ZodRawShape, + TArgs = Record, +>(server: McpServer, tool: ToolDefinition): void { if (tool.parameters) { - server.tool(tool.name, tool.description, tool.parameters, async (args: any) => { - return tool.execute(args); + server.tool(tool.name, tool.description, tool.parameters, async (args) => { + return tool.execute(args as unknown as TArgs); }); } else { server.tool(tool.name, tool.description, async () => { - return tool.execute({}); + return tool.execute({} as unknown as TArgs); }); } } diff --git a/tests/tools.test.ts b/tests/tools.test.ts index 0ecf039..c56f093 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -36,9 +36,21 @@ describe("Tool Utilities", () => { expect(response.content[0]?.text).toContain("unexpected failure string"); }); - test("registerTool registers tool with parameters on McpServer", () => { + test("OpenProjectNotFoundError retains OPENPROJECT_NOT_FOUND code when errorIdentifier is provided", () => { + const error = new OpenProjectNotFoundError("Project 99 not found", { + errorIdentifier: "urn:openproject-org:api:v3:errors:NotFound", + }); + expect(error.code).toBe("OPENPROJECT_NOT_FOUND"); + expect(error.errorIdentifier).toBe("urn:openproject-org:api:v3:errors:NotFound"); + + const response = formatToolError(error); + expect(response.isError).toBe(true); + expect(response.content[0]?.text).toBe("Error [OPENPROJECT_NOT_FOUND]: Project 99 not found"); + }); + + test("registerTool registers tool with parameters on McpServer", async () => { const server = new McpServer({ name: "test-server", version: "1.0.0" }); - const tool: ToolDefinition<{ id: z.ZodNumber }> = { + const tool: ToolDefinition<{ id: z.ZodNumber }, { id: number }> = { name: "test_tool_with_params", description: "A test tool with parameters", parameters: { id: z.number() }, @@ -47,10 +59,33 @@ describe("Tool Utilities", () => { }; registerTool(server, tool); - expect(tool.name).toBe("test_tool_with_params"); + + const registeredTools = ( + server as unknown as { + _registeredTools: Record< + string, + { + description?: string; + handler: (args: Record) => Promise<{ + content: Array<{ type: string; text: string }>; + isError?: boolean; + }>; + } + >; + } + )._registeredTools; + + const registered = registeredTools[tool.name]; + expect(registered).toBeDefined(); + expect(registered?.description).toBe("A test tool with parameters"); + + const executionResult = await registered?.handler({ id: 21 }); + expect(executionResult).toBeDefined(); + expect(executionResult?.isError).toBeUndefined(); + expect(JSON.parse(executionResult?.content[0]?.text ?? "{}")).toEqual({ doubled: 42 }); }); - test("registerTool registers parameterless tool on McpServer", () => { + test("registerTool registers parameterless tool on McpServer", async () => { const server = new McpServer({ name: "test-server", version: "1.0.0" }); const tool: ToolDefinition = { name: "test_tool_no_params", @@ -60,6 +95,29 @@ describe("Tool Utilities", () => { }; registerTool(server, tool); - expect(tool.name).toBe("test_tool_no_params"); + + const registeredTools = ( + server as unknown as { + _registeredTools: Record< + string, + { + description?: string; + handler: (args: Record) => Promise<{ + content: Array<{ type: string; text: string }>; + isError?: boolean; + }>; + } + >; + } + )._registeredTools; + + const registered = registeredTools[tool.name]; + expect(registered).toBeDefined(); + expect(registered?.description).toBe("A test tool without parameters"); + + const executionResult = await registered?.handler({}); + expect(executionResult).toBeDefined(); + expect(executionResult?.isError).toBeUndefined(); + expect(JSON.parse(executionResult?.content[0]?.text ?? "{}")).toEqual({ ok: true }); }); });