fix(agent): advertise null on optional MCP properties for GPT models

Author: bwachman

src/agent/tool-schema-normalize.test.ts

@@ -163,6 +163,78 @@ describe("normalizeToolDefinitionsForProvider", () => {
     }
   });
 
+  describe("optional MCP properties", () => {
+    const listIssues: ToolDefinition = {
+      name: "mcp__linear__list_issues",
+      description: "[linear] list issues",
+      inputSchema: {
+        type: "object",
+        properties: {
+          release: { type: "string" },
+          priority: { type: "number" },
+        },
+        required: ["release"],
+      },
+    };
+    const builtin: ToolDefinition = {
+      ...listIssues,
+      name: "list_issues",
+    };
+
+    test.each([
+      { providerName: "codex/plus", model: "gpt-5.6-terra" },
+      { providerName: "opencode-go", model: "gpt-5.1" },
+      { providerName: "codex", model: "codex-mini-latest" },
+    ])("$providerName $model advertises null on MCP tools only", (ctx) => {
+      const [mcp, own] = normalizeToolDefinitionsForProvider(
+        [listIssues, builtin],
+        ctx,
+      );
+      expect(mcp?.inputSchema).toEqual({
+        type: "object",
+        properties: {
+          release: { type: "string" },
+          priority: { type: ["number", "null"] },
+        },
+        required: ["release"],
+      });
+      expect(own).toBe(builtin);
+    });
+
+    test.each([
+      { providerName: "anthropic", model: "claude-sonnet-5-5" },
+      { providerName: "moonshot", model: "kimi-k2" },
+    ])("$providerName leaves MCP tools as they are", (ctx) => {
+      const defs = [listIssues];
+      expect(normalizeToolDefinitionsForProvider(defs, ctx)).toBe(defs);
+    });
+
+    test("grok sanitizes MCP tools without advertising null", () => {
+      const [out] = normalizeToolDefinitionsForProvider([listIssues], {
+        providerName: "xai/default-2",
+        model: "grok-4.6",
+      });
+      expect(out?.inputSchema).toEqual(listIssues.inputSchema);
+    });
+
+    test("returns the same array when no MCP property changes", () => {
+      const nullable: ToolDefinition = {
+        ...listIssues,
+        inputSchema: {
+          type: "object",
+          properties: { assignee: { type: ["string", "null"] } },
+        },
+      };
+      const defs = [nullable, builtin];
+      expect(
+        normalizeToolDefinitionsForProvider(defs, {
+          providerName: "openai",
+          model: "gpt-5.6",
+        }),
+      ).toBe(defs);
+    });
+  });
+
   test("grok sanitizes untyped properties and strips $schema", () => {
     const untyped: ToolDefinition = {
       name: "submit_result",

src/agent/tool-schema-normalize.ts

@@ -1,5 +1,8 @@
 import type { ToolDefinition } from "@intx/types/runtime";
+import { advertiseNullForOptionalProperties } from "../mcp/placeholder-args.js";
+import { isMcpToolName } from "../mcp/tool-name.js";
 import {
+  isGptProvider,
   isKimiLeafProvider,
   isMuseSparkLeafProvider,
   isXaiGrokLeafProvider,
@@ -317,33 +320,49 @@ function sanitizeToolDefForGrok(def: ToolDefinition): ToolDefinition {
   };
 }
 
+// GPT pads optional arguments with real values ("priority: 0") where it
+// cannot write null; the MCP handler drops the null it writes instead.
+function advertiseNullForMcpTool(def: ToolDefinition): ToolDefinition {
+  if (!isMcpToolName(def.name)) return def;
+  const inputSchema = advertiseNullForOptionalProperties(def.inputSchema);
+  return inputSchema === def.inputSchema ? def : { ...def, inputSchema };
+}
+
 function needsAcyclicPresentSchema(ctx: NormalizeToolDefsContext): boolean {
   return isKimiLeafProvider(ctx) || isMuseSparkLeafProvider(ctx);
 }
 
 /**
  * Family-gated wire rewrite of tool defs before they reach the
  * director / provider. Moonshot/kimi and Muse Spark get a non-recursive
- * `present` schema; Grok/xAI get schema sanitization; others pass through.
- * Does not alter runtime validation or the canonical `presentDefinition`.
+ * `present` schema; Grok/xAI get schema sanitization; GPT gets `null` on
+ * optional MCP properties; others pass through. Does not alter runtime
+ * validation or the canonical `presentDefinition`. Returns `defs` itself
+ * when no def changes, so an unchanged toolset keeps its cache identity.
  *
- * Call at every advertise path that may include `present`; sub-agent
- * toolsets omit it, so rewriting is a no-op unless one appears.
+ * Call wherever a tools array is built for the provider. Text that quotes
+ * a schema back to the model (tool_search cards) keeps the canonical
+ * definition.
  */
 export function normalizeToolDefinitionsForProvider(
   defs: readonly ToolDefinition[],
   ctx: NormalizeToolDefsContext,
 ): ToolDefinition[] {
   const acyclicPresent = needsAcyclicPresentSchema(ctx);
   const grok = isXaiGrokLeafProvider(ctx);
-  if (!acyclicPresent && !grok) {
+  const gpt = isGptProvider(ctx);
+  if (!acyclicPresent && !grok && !gpt) {
     return defs as ToolDefinition[];
   }
-  return defs.map((def) => {
+  const out = defs.map((def) => {
     let next = def;
     if (acyclicPresent && def.name === "present")
       next = rewritePresentAcyclic(next);
     if (grok) next = sanitizeToolDefForGrok(next);
+    if (gpt) next = advertiseNullForMcpTool(next);
     return next;
   });
+  return out.every((def, i) => def === defs[i])
+    ? (defs as ToolDefinition[])
+    : out;
 }

src/mcp/placeholder-args.test.ts

@@ -1,5 +1,8 @@
 import { describe, expect, test } from "bun:test";
-import { stripPlaceholderArguments } from "./placeholder-args.js";
+import {
+  advertiseNullForOptionalProperties,
+  stripPlaceholderArguments,
+} from "./placeholder-args.js";
 
 const schema = {
   type: "object",
@@ -213,3 +216,173 @@ describe("stripPlaceholderArguments with nullMeansUnset", () => {
     ).toEqual({ extra: null });
   });
 });
+
+function propertiesOf(
+  inputSchema: Record<string, unknown>,
+): Record<string, unknown> {
+  const properties = inputSchema["properties"];
+  if (typeof properties !== "object" || properties === null)
+    throw new Error("schema has no properties");
+  return Object.fromEntries(Object.entries(properties));
+}
+
+describe("advertiseNullForOptionalProperties", () => {
+  // Shapes from Linear's list_issues and save_issue.
+  const linear = {
+    type: "object",
+    properties: {
+      id: { type: "string" },
+      priority: { type: "number", description: "0=None, 1=Urgent" },
+      customView: { type: "string", minLength: 1 },
+      orderBy: { type: "string", enum: ["createdAt", "updatedAt"] },
+      fields: { type: "array", items: { type: "string" } },
+      assignee: { type: ["string", "null"] },
+      slaType: {
+        anyOf: [{ enum: ["all", "onlyBusinessDays"] }, { type: "null" }],
+      },
+      label: { oneOf: [{ type: "string" }, { type: "number" }] },
+      filter: { type: "object", properties: { state: { type: "string" } } },
+      scope: { type: "string", const: "all" },
+      anything: { description: "untyped" },
+    },
+    required: ["id"],
+    additionalProperties: false,
+  };
+
+  test("adds null to optional properties that reject it", () => {
+    const out = advertiseNullForOptionalProperties(linear);
+    expect(out["properties"]).toEqual({
+      ...linear.properties,
+      priority: { type: ["number", "null"], description: "0=None, 1=Urgent" },
+      customView: { type: ["string", "null"], minLength: 1 },
+      orderBy: {
+        type: ["string", "null"],
+        enum: ["createdAt", "updatedAt", null],
+      },
+      fields: { type: ["array", "null"], items: { type: "string" } },
+      label: {
+        oneOf: [{ type: "string" }, { type: "number" }, { type: "null" }],
+      },
+      filter: {
+        type: ["object", "null"],
+        properties: { state: { type: "string" } },
+      },
+    });
+    expect(out["required"]).toEqual(["id"]);
+    expect(out["additionalProperties"]).toBe(false);
+  });
+
+  test("leaves required, already-nullable, untyped, and const properties as they are", () => {
+    const before = propertiesOf(linear);
+    const after = propertiesOf(advertiseNullForOptionalProperties(linear));
+    for (const key of ["id", "assignee", "slaType", "scope", "anything"])
+      expect(after[key]).toBe(before[key]);
+  });
+
+  test("leaves shapes the flat form cannot widen validly as they are", () => {
+    const ambiguous = {
+      type: "object",
+      properties: {
+        due: {
+          type: "string",
+          oneOf: [{ format: "date" }, { format: "time" }],
+        },
+        id: { type: "string", allOf: [{ type: "string" }] },
+        code: { type: "string", not: { const: "x" } },
+        ref: { type: "string", $ref: "#/$defs/S" },
+        branchy: {
+          type: "string",
+          oneOf: [{ type: "string" }, { minLength: 1 }],
+        },
+        conditional: { type: "string", if: true, then: { type: "string" } },
+        dynamic: { type: "string", $dynamicRef: "#/$defs/S" },
+        exact: { const: "all" },
+        openAPI: {
+          type: "object",
+          oneOf: [{ type: "string", nullable: true }, { type: "number" }],
+        },
+        strict: { type: "string", nullable: false },
+      },
+    };
+    expect(advertiseNullForOptionalProperties(ambiguous)).toBe(ambiguous);
+  });
+
+  test("adds a null branch to anyOf whatever its other branches hold", () => {
+    const opaque = {
+      type: "object",
+      properties: {
+        either: { type: "string", anyOf: [{ $ref: "#/$defs/S" }] },
+        nested: { type: "string", anyOf: [{ allOf: [{ type: "string" }] }] },
+      },
+    };
+    expect(propertiesOf(advertiseNullForOptionalProperties(opaque))).toEqual({
+      either: {
+        type: ["string", "null"],
+        anyOf: [{ $ref: "#/$defs/S" }, { type: "null" }],
+      },
+      nested: {
+        type: ["string", "null"],
+        anyOf: [{ allOf: [{ type: "string" }] }, { type: "null" }],
+      },
+    });
+  });
+
+  test("does not repeat a null that a keyword already lists", () => {
+    const partial = {
+      type: "object",
+      properties: {
+        state: { type: "string", enum: ["open", null] },
+        view: {
+          type: "string",
+          anyOf: [{ type: "string", minLength: 1 }, { type: "null" }],
+        },
+      },
+    };
+    expect(propertiesOf(advertiseNullForOptionalProperties(partial))).toEqual({
+      state: { type: ["string", "null"], enum: ["open", null] },
+      view: {
+        type: ["string", "null"],
+        anyOf: [{ type: "string", minLength: 1 }, { type: "null" }],
+      },
+    });
+  });
+
+  test("leaves the input schema intact", () => {
+    const before = structuredClone(linear);
+    advertiseNullForOptionalProperties(linear);
+    expect(linear).toEqual(before);
+  });
+
+  test("returns the same object when no property changes", () => {
+    const nullable = {
+      type: "object",
+      properties: { assignee: { type: ["string", "null"] } },
+    };
+    expect(advertiseNullForOptionalProperties(nullable)).toBe(nullable);
+    const empty = { type: "object", properties: {} };
+    expect(advertiseNullForOptionalProperties(empty)).toBe(empty);
+    const bare = { type: "object" };
+    expect(advertiseNullForOptionalProperties(bare)).toBe(bare);
+    const unusable = { type: "object", required: "id" };
+    expect(advertiseNullForOptionalProperties(unusable)).toBe(unusable);
+  });
+
+  test("adds null only where the strip rule drops it again", () => {
+    const before = propertiesOf(schema);
+    const after = propertiesOf(advertiseNullForOptionalProperties(schema));
+    const added = Object.keys(before).filter(
+      (key) => after[key] !== before[key],
+    );
+    const dropped = Object.keys(before).filter(
+      (key) =>
+        !(key in stripPlaceholderArguments({ [key]: null }, schema, keepNull)),
+    );
+    expect(added).not.toEqual([]);
+    // A const cannot be widened to null, so its null is dropped unadvertised.
+    expect(dropped).toEqual(
+      Object.keys(before).filter(
+        (key) => added.includes(key) || key === "scope",
+      ),
+    );
+  });
+});