Merge pull request #480 from olaservo/fix-required

Fix fundamental schema issue with required fields defined incorrectly on JsonSchemaType

client/src/utils/__tests__/schemaUtils.test.ts

@@ -11,72 +11,68 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js";
 
 describe("generateDefaultValue", () => {
   test("generates default string", () => {
-    expect(generateDefaultValue({ type: "string", required: true })).toBe("");
+    const parentSchema = { type: "object" as const, required: ["testProp"] };
+    expect(
+      generateDefaultValue({ type: "string" }, "testProp", parentSchema),
+    ).toBe("");
   });
 
   test("generates default number", () => {
-    expect(generateDefaultValue({ type: "number", required: true })).toBe(0);
+    const parentSchema = { type: "object" as const, required: ["testProp"] };
+    expect(
+      generateDefaultValue({ type: "number" }, "testProp", parentSchema),
+    ).toBe(0);
   });
 
   test("generates default integer", () => {
-    expect(generateDefaultValue({ type: "integer", required: true })).toBe(0);
+    const parentSchema = { type: "object" as const, required: ["testProp"] };
+    expect(
+      generateDefaultValue({ type: "integer" }, "testProp", parentSchema),
+    ).toBe(0);
   });
 
   test("generates default boolean", () => {
-    expect(generateDefaultValue({ type: "boolean", required: true })).toBe(
-      false,
-    );
+    const parentSchema = { type: "object" as const, required: ["testProp"] };
+    expect(
+      generateDefaultValue({ type: "boolean" }, "testProp", parentSchema),
+    ).toBe(false);
   });
 
   test("generates default array", () => {
-    expect(generateDefaultValue({ type: "array", required: true })).toEqual([]);
+    expect(generateDefaultValue({ type: "array" })).toEqual([]);
   });
 
   test("generates default empty object", () => {
-    expect(generateDefaultValue({ type: "object", required: true })).toEqual(
-      {},
-    );
+    expect(generateDefaultValue({ type: "object" })).toEqual({});
   });
 
   test("generates default null for unknown types", () => {
     // @ts-expect-error Testing with invalid type
-    expect(generateDefaultValue({ type: "unknown", required: true })).toBe(
-      null,
-    );
+    expect(generateDefaultValue({ type: "unknown" })).toBe(undefined);
   });
 
   test("generates empty array for non-required array", () => {
-    expect(generateDefaultValue({ type: "array", required: false })).toEqual(
-      [],
-    );
+    expect(generateDefaultValue({ type: "array" })).toEqual([]);
   });
 
   test("generates empty object for non-required object", () => {
-    expect(generateDefaultValue({ type: "object", required: false })).toEqual(
-      {},
-    );
+    expect(generateDefaultValue({ type: "object" })).toEqual({});
   });
 
-  test("generates null for non-required primitive types", () => {
-    expect(generateDefaultValue({ type: "string", required: false })).toBe(
-      undefined,
-    );
-    expect(generateDefaultValue({ type: "number", required: false })).toBe(
-      undefined,
-    );
-    expect(generateDefaultValue({ type: "boolean", required: false })).toBe(
-      undefined,
-    );
+  test("generates undefined for non-required primitive types", () => {
+    expect(generateDefaultValue({ type: "string" })).toBe(undefined);
+    expect(generateDefaultValue({ type: "number" })).toBe(undefined);
+    expect(generateDefaultValue({ type: "boolean" })).toBe(undefined);
   });
 
   test("generates object with properties", () => {
     const schema: JsonSchemaType = {
       type: "object",
-      required: true,
+      required: ["name", "age", "isActive"],
       properties: {
-        name: { type: "string", required: true },
-        age: { type: "number", required: true },
-        isActive: { type: "boolean", required: true },
+        name: { type: "string" },
+        age: { type: "number" },
+        isActive: { type: "boolean" },
       },
     };
     expect(generateDefaultValue(schema)).toEqual({
@@ -89,18 +85,18 @@ describe("generateDefaultValue", () => {
   test("handles nested objects", () => {
     const schema: JsonSchemaType = {
       type: "object",
-      required: true,
+      required: ["user"],
       properties: {
         user: {
           type: "object",
-          required: true,
+          required: ["name", "address"],
           properties: {
-            name: { type: "string", required: true },
+            name: { type: "string" },
             address: {
               type: "object",
-              required: true,
+              required: ["city"],
               properties: {
-                city: { type: "string", required: true },
+                city: { type: "string" },
               },
             },
           },

client/src/utils/jsonUtils.ts

@@ -17,7 +17,7 @@ export type JsonSchemaType = {
     | "object"
     | "null";
   description?: string;
-  required?: boolean;
+  required?: string[];
   default?: JsonValue;
   properties?: Record<string, JsonSchemaType>;
   items?: JsonSchemaType;

client/src/utils/schemaUtils.ts

@@ -81,46 +81,70 @@ export function hasOutputSchema(toolName: string): boolean {
 /**
  * Generates a default value based on a JSON schema type
  * @param schema The JSON schema definition
- * @returns A default value matching the schema type, or null for non-required fields
+ * @param propertyName Optional property name for checking if it's required in parent schema
+ * @param parentSchema Optional parent schema to check required array
+ * @returns A default value matching the schema type
  */
-export function generateDefaultValue(schema: JsonSchemaType): JsonValue {
-  if ("default" in schema) {
+export function generateDefaultValue(
+  schema: JsonSchemaType,
+  propertyName?: string,
+  parentSchema?: JsonSchemaType,
+): JsonValue {
+  if ("default" in schema && schema.default !== undefined) {
     return schema.default;
   }
 
-  if (!schema.required) {
-    if (schema.type === "array") return [];
-    if (schema.type === "object") return {};
-    return undefined;
-  }
+  // Check if this property is required in the parent schema
+  const isRequired =
+    propertyName && parentSchema
+      ? isPropertyRequired(propertyName, parentSchema)
+      : false;
 
   switch (schema.type) {
     case "string":
-      return "";
+      return isRequired ? "" : undefined;
     case "number":
     case "integer":
-      return 0;
+      return isRequired ? 0 : undefined;
     case "boolean":
-      return false;
+      return isRequired ? false : undefined;
     case "array":
       return [];
     case "object": {
       if (!schema.properties) return {};
 
       const obj: JsonObject = {};
-      Object.entries(schema.properties)
-        .filter(([, prop]) => prop.required)
-        .forEach(([key, prop]) => {
-          const value = generateDefaultValue(prop);
-          obj[key] = value;
-        });
+      // Only include properties that are required according to the schema's required array
+      Object.entries(schema.properties).forEach(([key, prop]) => {
+        if (isPropertyRequired(key, schema)) {
+          const value = generateDefaultValue(prop, key, schema);
+          if (value !== undefined) {
+            obj[key] = value;
+          }
+        }
+      });
       return obj;
     }
-    default:
+    case "null":
       return null;
+    default:
+      return undefined;
   }
 }
 
+/**
+ * Helper function to check if a property is required in a schema
+ * @param propertyName The name of the property to check
+ * @param schema The parent schema containing the required array
+ * @returns true if the property is required, false otherwise
+ */
+export function isPropertyRequired(
+  propertyName: string,
+  schema: JsonSchemaType,
+): boolean {
+  return schema.required?.includes(propertyName) ?? false;
+}
+
 /**
  * Formats a field key into a human-readable label
  * @param key The field key to format