Skip to content

Commit ade5be1

Browse files
TheGreatAxiosTheGreatAxios
authored
Merge pull request #1568 from corbitsdev/cl-11344-strip-empty-placeholder-arguments-from-mcp-tool-calls
fix(mcp): drop placeholder arguments the tool schema would reject
2 parents c3a8cb6 + d10a8fd commit ade5be1

4 files changed

Lines changed: 315 additions & 1 deletion

File tree

‎e2e/mcp-late-dispatch.test.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,4 +369,69 @@ describe("integration — late MCP dispatch", () => {
369369
});
370370
},
371371
);
372+
373+
test.serial(
374+
"padded optional MCP arguments reach the server minus the values its schema rejects",
375+
async () => {
376+
const received: Record<string, unknown>[] = [];
377+
await withSession(async (session) => {
378+
session.toolset.dynamicRunner.addTools(
379+
mcpClientTools(
380+
linearClient({
381+
inputSchema: {
382+
type: "object",
383+
properties: {
384+
customView: { type: "string", minLength: 1 },
385+
cursor: { type: "string" },
386+
query: { type: "string" },
387+
team: { type: "string" },
388+
fields: { type: "array", items: { type: "string" } },
389+
priority: { type: "number" },
390+
assignee: { type: ["string", "null"] },
391+
release: { type: "string" },
392+
},
393+
},
394+
call: async (_toolName, args) => {
395+
received.push(args);
396+
return "ISSUE-1";
397+
},
398+
}),
399+
),
400+
);
401+
// The padding gpt-5.6-terra sent on a real release lookup.
402+
replyScript(
403+
session,
404+
[
405+
{
406+
name: LATE_MCP,
407+
args: {
408+
customView: " ",
409+
cursor: "",
410+
query: "",
411+
team: "",
412+
fields: [],
413+
priority: 0,
414+
assignee: null,
415+
release: "v0345-ef53e73667fa",
416+
},
417+
},
418+
],
419+
"listed",
420+
);
421+
422+
await runUntilDone(session, "list the issues in this release");
423+
});
424+
expect(received).toEqual([
425+
{
426+
cursor: "",
427+
query: "",
428+
team: "",
429+
fields: [],
430+
priority: 0,
431+
assignee: null,
432+
release: "v0345-ef53e73667fa",
433+
},
434+
]);
435+
},
436+
);
372437
});

‎src/mcp/placeholder-args.test.ts‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { stripPlaceholderArguments } from "./placeholder-args.js";
3+
4+
const schema = {
5+
type: "object",
6+
properties: {
7+
id: { type: "string" },
8+
query: { type: "string" },
9+
note: { type: "string", minLength: 0 },
10+
customView: { type: "string", minLength: 1 },
11+
view: { anyOf: [{ type: "string", minLength: 1 }, { type: "null" }] },
12+
label: { oneOf: [{ type: "string", minLength: 1 }, { type: "string" }] },
13+
state: { type: "string", enum: ["open", "closed"] },
14+
stage: { type: "string", enum: ["", "planned"] },
15+
scope: { type: "string", const: "all" },
16+
slug: { type: "string", pattern: "^[a-z]+$" },
17+
suffix: { type: "string", pattern: "[a-z]*" },
18+
malformed: { type: "string", pattern: "(" },
19+
fields: { type: "array", items: { type: "string" } },
20+
labels: { type: "array", items: { type: "string" }, minItems: 1 },
21+
team: { type: "string" },
22+
assignee: { type: ["string", "null"] },
23+
parentId: { anyOf: [{ type: "string" }, { type: "null" }] },
24+
cycle: { type: "string", nullable: true },
25+
priority: { type: "number" },
26+
includeArchived: { type: "boolean" },
27+
anything: {},
28+
},
29+
required: ["id"],
30+
};
31+
32+
describe("stripPlaceholderArguments", () => {
33+
test("forwards empty and whitespace strings the property schema permits", () => {
34+
const args = { query: "", note: "", team: " ", suffix: "", stage: "" };
35+
expect(stripPlaceholderArguments(args, schema)).toEqual(args);
36+
});
37+
38+
test("drops an empty or whitespace string where minLength is at least 1", () => {
39+
expect(
40+
stripPlaceholderArguments({ customView: " ", query: "x" }, schema),
41+
).toEqual({ query: "x" });
42+
expect(stripPlaceholderArguments({ customView: "" }, schema)).toEqual({});
43+
});
44+
45+
test("drops an empty string that an enum, const, or pattern rejects", () => {
46+
expect(
47+
stripPlaceholderArguments({ state: "", scope: " ", slug: "" }, schema),
48+
).toEqual({});
49+
});
50+
51+
test("forwards an empty string when the pattern does not compile", () => {
52+
expect(stripPlaceholderArguments({ malformed: "" }, schema)).toEqual({
53+
malformed: "",
54+
});
55+
});
56+
57+
test("drops an empty array only where minItems is at least 1", () => {
58+
expect(
59+
stripPlaceholderArguments({ fields: [], labels: [] }, schema),
60+
).toEqual({ fields: [] });
61+
});
62+
63+
test("drops an empty value the property type does not admit", () => {
64+
expect(
65+
stripPlaceholderArguments(
66+
{ priority: "", includeArchived: " ", query: [] },
67+
schema,
68+
),
69+
).toEqual({});
70+
});
71+
72+
test("drops null only where the property schema disallows it", () => {
73+
expect(
74+
stripPlaceholderArguments({ team: null, priority: null }, schema),
75+
).toEqual({});
76+
const nullable = {
77+
assignee: null,
78+
parentId: null,
79+
cycle: null,
80+
anything: null,
81+
};
82+
expect(stripPlaceholderArguments(nullable, schema)).toEqual(nullable);
83+
});
84+
85+
test("drops a composite value only when every branch rejects it", () => {
86+
expect(stripPlaceholderArguments({ view: " " }, schema)).toEqual({});
87+
expect(stripPlaceholderArguments({ view: null }, schema)).toEqual({
88+
view: null,
89+
});
90+
expect(stripPlaceholderArguments({ label: "" }, schema)).toEqual({
91+
label: "",
92+
});
93+
});
94+
95+
test("never changes required arguments", () => {
96+
expect(stripPlaceholderArguments({ id: "" }, schema)).toEqual({ id: "" });
97+
expect(stripPlaceholderArguments({ id: null }, schema)).toEqual({
98+
id: null,
99+
});
100+
});
101+
102+
test("keeps zero and false", () => {
103+
const args = { priority: 0, includeArchived: false };
104+
expect(stripPlaceholderArguments(args, schema)).toEqual(args);
105+
});
106+
107+
test("keeps meaningful values and leaves the caller's object intact", () => {
108+
const args = { query: "release", fields: ["title"], customView: "" };
109+
expect(stripPlaceholderArguments(args, schema)).toEqual({
110+
query: "release",
111+
fields: ["title"],
112+
});
113+
expect(args).toEqual({
114+
query: "release",
115+
fields: ["title"],
116+
customView: "",
117+
});
118+
});
119+
120+
test("forwards a key the schema does not declare", () => {
121+
expect(
122+
stripPlaceholderArguments({ id: "CL-1", extra: "" }, schema),
123+
).toEqual({ id: "CL-1", extra: "" });
124+
});
125+
126+
test("returns missing or non-object arguments as-is", () => {
127+
expect(stripPlaceholderArguments(null, schema)).toBeNull();
128+
expect(stripPlaceholderArguments(undefined, schema)).toBeUndefined();
129+
expect(stripPlaceholderArguments("query", schema)).toBe("query");
130+
const list = [""];
131+
expect(stripPlaceholderArguments(list, schema)).toBe(list);
132+
});
133+
134+
test("forwards arguments unchanged when the schema shape is unusable", () => {
135+
const args = { query: "" };
136+
expect(
137+
stripPlaceholderArguments(args, { type: "object", required: "query" }),
138+
).toEqual(args);
139+
});
140+
});

‎src/mcp/placeholder-args.ts‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import { type } from "arktype";
2+
3+
const ObjectSchema = type({
4+
"properties?": "Record<string, unknown>",
5+
"required?": "string[]",
6+
});
7+
8+
const Arguments = type("Record<string, unknown>");
9+
10+
const PropertySchema = type({
11+
"type?": "string | string[]",
12+
"nullable?": "boolean",
13+
"enum?": "unknown[]",
14+
"const?": "unknown",
15+
"minLength?": "number",
16+
"minItems?": "number",
17+
"pattern?": "string",
18+
"anyOf?": "unknown[]",
19+
"oneOf?": "unknown[]",
20+
});
21+
22+
type PropertySchema = typeof PropertySchema.infer;
23+
24+
function typeName(value: unknown): string {
25+
if (value === null) return "null";
26+
if (Array.isArray(value)) return "array";
27+
return typeof value;
28+
}
29+
30+
function admitsType(schema: PropertySchema, value: unknown): boolean {
31+
if (schema.type === undefined) return true;
32+
if (value === null && schema.nullable === true) return true;
33+
const allowed = Array.isArray(schema.type) ? schema.type : [schema.type];
34+
return allowed.includes(typeName(value));
35+
}
36+
37+
// An uncompilable pattern proves nothing about the value, so it never rejects.
38+
function matchesPattern(pattern: string, value: string): boolean {
39+
try {
40+
return new RegExp(pattern, "u").test(value);
41+
} catch {
42+
return true;
43+
}
44+
}
45+
46+
// Only the constraints an empty value can violate are checked; anything the
47+
// schema does not express is left for the server to judge.
48+
function admits(property: unknown, value: unknown): boolean {
49+
const schema = PropertySchema(property);
50+
if (schema instanceof type.errors) return true;
51+
const branches = [...(schema.anyOf ?? []), ...(schema.oneOf ?? [])];
52+
if (branches.length > 0 && !branches.some((branch) => admits(branch, value)))
53+
return false;
54+
if (schema.enum !== undefined && !schema.enum.includes(value)) return false;
55+
if (schema.const !== undefined && schema.const !== value) return false;
56+
if (!admitsType(schema, value)) return false;
57+
if (typeof value === "string") {
58+
if ((schema.minLength ?? 0) > value.length) return false;
59+
if (schema.pattern !== undefined && !matchesPattern(schema.pattern, value))
60+
return false;
61+
}
62+
if (Array.isArray(value) && (schema.minItems ?? 0) > value.length)
63+
return false;
64+
return true;
65+
}
66+
67+
function isRejectedPlaceholder(value: unknown, property: unknown): boolean {
68+
// Servers trim before checking length, so whitespace is judged as "".
69+
if (typeof value === "string")
70+
return value.trim() === "" && !admits(property, "");
71+
if (Array.isArray(value)) return value.length === 0 && !admits(property, []);
72+
return value === null && !admits(property, null);
73+
}
74+
75+
/**
76+
* Drops optional arguments a model filled with an empty placeholder (`""`,
77+
* whitespace, `[]`, or `null`) that the property's own schema would reject,
78+
* so one padded field does not fail the whole call. Empty values the schema
79+
* permits, required arguments, and any other value are forwarded unchanged.
80+
*/
81+
export function stripPlaceholderArguments(
82+
args: Record<string, unknown>,
83+
inputSchema: Record<string, unknown>,
84+
): Record<string, unknown>;
85+
export function stripPlaceholderArguments(
86+
args: unknown,
87+
inputSchema: Record<string, unknown>,
88+
): unknown;
89+
export function stripPlaceholderArguments(
90+
args: unknown,
91+
inputSchema: Record<string, unknown>,
92+
): unknown {
93+
const parsedArgs = Arguments(args);
94+
if (parsedArgs instanceof type.errors || Array.isArray(parsedArgs))
95+
return args;
96+
const schema = ObjectSchema(inputSchema);
97+
if (schema instanceof type.errors) return args;
98+
const required = new Set(schema.required ?? []);
99+
return Object.fromEntries(
100+
Object.entries(parsedArgs).filter(
101+
([key, value]) =>
102+
required.has(key) ||
103+
!isRejectedPlaceholder(value, schema.properties?.[key]),
104+
),
105+
);
106+
}

‎src/mcp/plugin.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
type MCPClient,
1919
type MCPContentBlock,
2020
} from "./client.js";
21+
import { stripPlaceholderArguments } from "./placeholder-args.js";
2122
import { mcpToolName } from "./tool-name.js";
2223
import { LOG_NAMESPACE_ROOT } from "../branding.js";
2324

@@ -107,9 +108,11 @@ export function mcpClientTools(
107108
signal: AbortSignal,
108109
): Promise<ToolResult> => {
109110
try {
111+
// Some models pad every optional field with empty values; one the
112+
// property schema rejects (customView minLength) fails the call.
110113
const envelope = await client.callResult(
111114
tool.name,
112-
call.arguments,
115+
stripPlaceholderArguments(call.arguments, tool.inputSchema),
113116
signal,
114117
);
115118
const authorizedBlocks = applyPolicyToBlocks(envelope.blocks);

0 commit comments

Comments
 (0)