fix(tools): address review findings on error codes, registerTool assertions, and strict typing

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";
   }
 }

src/tools/common.ts

@@ -53,25 +53,31 @@ export function formatToolError(error: unknown): McpToolResponse {
 /**
  * Definition interface for OpenProject MCP tools.
  */
-export interface ToolDefinition<TShape extends ZodRawShape = ZodRawShape> {
+export interface ToolDefinition<
+  TShape extends ZodRawShape = ZodRawShape,
+  TArgs = Record<string, unknown>,
+> {
   name: string;
   description: string;
   parameters?: TShape;
   readOnly: boolean;
-  execute: (args: any) => Promise<McpToolResponse>;
+  execute: (args: TArgs) => Promise<McpToolResponse>;
 }
 
 /**
  * Registers a ToolDefinition instance on an McpServer instance.
  */
-export function registerTool(server: McpServer, tool: ToolDefinition<any>): void {
+export function registerTool<
+  TShape extends ZodRawShape = ZodRawShape,
+  TArgs = Record<string, unknown>,
+>(server: McpServer, tool: ToolDefinition<TShape, TArgs>): void {
   if (tool.parameters) {
-    server.tool(tool.name, tool.description, tool.parameters, async (args: any) => {
+    server.tool(tool.name, tool.description, tool.parameters, async (args) => {
-      return tool.execute(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);
     });
   }
 }

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<string, unknown>) => 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<string, unknown>) => 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 });
   });
 });