From 6b8045111bc5c46dc6ae3a886e7d010dddd7f1ba Mon Sep 17 00:00:00 2001
From: Lucas Winther <lucwin@ece.au.dk>
Date: Sun, 6 Sep 2026 18:37:21 +0200
Subject: [PATCH] fix(tools): address final code review findings on validation,
 concurrency, and testing

---
 src/tools/index.ts    |  6 ++++--
 src/tools/projects.ts | 10 ++++++++--
 src/tools/queries.ts  | 12 +++++++-----
 tests/tools.test.ts   | 39 ++++++++++++++++++++++++++++++++++++++-
 4 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/src/tools/index.ts b/src/tools/index.ts
index 313d03d..ba75ff3 100644
--- a/src/tools/index.ts
+++ b/src/tools/index.ts
@@ -30,6 +30,7 @@ export const allTools: AnyToolDefinition[] = [
 
 export interface RegisterToolsOptions {
   readOnly?: boolean;
+  tools?: AnyToolDefinition[];
 }
 
 /**
@@ -41,12 +42,13 @@ export function registerAllTools(
   options?: RegisterToolsOptions
 ): void {
   const isReadOnly = options?.readOnly ?? false;
+  const toolsToRegister = options?.tools ?? allTools;
 
-  for (const tool of allTools) {
+  for (const tool of toolsToRegister) {
     // If server is in read-only mode, skip any non-read-only tools
     if (isReadOnly && !tool.readOnly) {
       continue;
     }
-    registerTool(server, tool);
+    registerTool(server, tool as unknown as ToolDefinition);
   }
 }
diff --git a/src/tools/projects.ts b/src/tools/projects.ts
index 59e4f98..88e704a 100644
--- a/src/tools/projects.ts
+++ b/src/tools/projects.ts
@@ -47,7 +47,13 @@ export async function handleListProjects(
   try {
     let filters: FilterElement[] | undefined;
     if (args.filters) {
-      filters = JSON.parse(args.filters) as FilterElement[];
+      const parsed = JSON.parse(args.filters);
+      if (!Array.isArray(parsed)) {
+        return formatToolError(
+          new Error("filters parameter must be a JSON array string")
+        );
+      }
+      filters = parsed as FilterElement[];
     }
 
     const result = await listProjects({
@@ -71,7 +77,7 @@ export async function handleListProjects(
 
 export const getProjectShape = {
   projectId: z
-    .union([z.number().int().positive(), z.string()])
+    .union([z.number().int().positive(), z.string().min(1)])
     .describe("Numeric project ID or string identifier/slug"),
 };
 
diff --git a/src/tools/queries.ts b/src/tools/queries.ts
index 75a6e98..ae2c14f 100644
--- a/src/tools/queries.ts
+++ b/src/tools/queries.ts
@@ -93,11 +93,13 @@ export async function handleGetQuery(
   args: GetQueryArgs
 ): Promise<McpToolResponse> {
   try {
-    const query = await getQuery(args.queryId);
-    const results = await getQueryResults(args.queryId, {
-      pageSize: args.pageSize,
-      offset: args.offset,
-    });
+    const [query, results] = await Promise.all([
+      getQuery(args.queryId),
+      getQueryResults(args.queryId, {
+        pageSize: args.pageSize,
+        offset: args.offset,
+      }),
+    ]);
 
     return formatToolSuccess({
       query,
diff --git a/tests/tools.test.ts b/tests/tools.test.ts
index 4315d04..3ef605e 100644
--- a/tests/tools.test.ts
+++ b/tests/tools.test.ts
@@ -252,6 +252,17 @@ describe("Project Tools", () => {
     expect(response.content[0]?.text).toContain("Error");
   });
 
+  test("handleListProjects catches non-array JSON filters and returns error response", async () => {
+    const response = await runWithContext(
+      { client: dummyClient, isReadOnly: false },
+      () => handleListProjects({ filters: '{"not":"an array"}' })
+    );
+    expect(response.isError).toBe(true);
+    expect(response.content[0]?.text).toContain(
+      "filters parameter must be a JSON array string"
+    );
+  });
+
   test("getProject validates schema and retrieves single project", async () => {
     const schema = z.object(getProjectShape);
     const parsed = schema.parse({ projectId: 4 });
@@ -266,6 +277,13 @@ describe("Project Tools", () => {
     expect(result.identifier).toBe("mcp-test-project");
   });
 
+  test("getProject rejects empty string projectId", () => {
+    const schema = z.object(getProjectShape);
+    expect(() => schema.parse({ projectId: "" })).toThrow();
+    expect(() => schema.parse({ projectId: 0 })).toThrow();
+    expect(() => schema.parse({ projectId: -1 })).toThrow();
+  });
+
   test("getProject catches errors and formats error response", async () => {
     const failingClient = {
       get: async () => {
@@ -977,7 +995,20 @@ describe("Tool Registry", () => {
 
   test("registerAllTools filters out non-readOnly tools when readOnly is true", () => {
     const server = new McpServer({ name: "test-mcp", version: "1.0.0" });
-    registerAllTools(server, { readOnly: true });
+    const syntheticMutatingTool: ToolDefinition = {
+      name: "openproject_synthetic_mutating_tool",
+      description: "A synthetic mutating tool for testing readOnly filtering",
+      readOnly: false,
+      execute: async () => formatToolSuccess({ mutated: true }),
+    };
+
+    registerAllTools(server, {
+      readOnly: true,
+      tools: [
+        ...allTools,
+        syntheticMutatingTool as unknown as AnyToolDefinition,
+      ],
+    });
 
     const registeredTools = (
       server as unknown as {
@@ -985,7 +1016,13 @@ describe("Tool Registry", () => {
       }
     )._registeredTools;
 
+    expect(
+      registeredTools["openproject_synthetic_mutating_tool"]
+    ).toBeUndefined();
     expect(Object.keys(registeredTools)).toHaveLength(10);
+    for (const tool of allTools) {
+      expect(registeredTools[tool.name]).toBeDefined();
+    }
   });
 });