Skip to content

Commit 321e32a

Browse files
bwachmanbwachman
committed
fix(mcp): drop null from optional arguments of read-only tools
1 parent 77ef1bd commit 321e32a

6 files changed

Lines changed: 211 additions & 41 deletions

File tree

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@ describe("integration — late MCP dispatch", () => {
371371
);
372372

373373
test.serial(
374-
"padded optional MCP arguments reach the server minus the values its schema rejects",
374+
"padded optional arguments to a read MCP tool reach the server without rejected placeholders or null",
375375
async () => {
376376
const received: Record<string, unknown>[] = [];
377377
await withSession(async (session) => {
@@ -428,7 +428,6 @@ describe("integration — late MCP dispatch", () => {
428428
team: "",
429429
fields: [],
430430
priority: 0,
431-
assignee: null,
432431
release: "v0345-ef53e73667fa",
433432
},
434433
]);

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

Lines changed: 98 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -29,34 +29,49 @@ const schema = {
2929
required: ["id"],
3030
};
3131

32+
const keepNull = { nullMeansUnset: false };
33+
const dropNull = { nullMeansUnset: true };
34+
3235
describe("stripPlaceholderArguments", () => {
3336
test("forwards empty and whitespace strings the property schema permits", () => {
3437
const args = { query: "", note: "", team: " ", suffix: "", stage: "" };
35-
expect(stripPlaceholderArguments(args, schema)).toEqual(args);
38+
expect(stripPlaceholderArguments(args, schema, keepNull)).toEqual(args);
3639
});
3740

3841
test("drops an empty or whitespace string where minLength is at least 1", () => {
3942
expect(
40-
stripPlaceholderArguments({ customView: " ", query: "x" }, schema),
43+
stripPlaceholderArguments(
44+
{ customView: " ", query: "x" },
45+
schema,
46+
keepNull,
47+
),
4148
).toEqual({ query: "x" });
42-
expect(stripPlaceholderArguments({ customView: "" }, schema)).toEqual({});
49+
expect(
50+
stripPlaceholderArguments({ customView: "" }, schema, keepNull),
51+
).toEqual({});
4352
});
4453

4554
test("drops an empty string that an enum, const, or pattern rejects", () => {
4655
expect(
47-
stripPlaceholderArguments({ state: "", scope: " ", slug: "" }, schema),
56+
stripPlaceholderArguments(
57+
{ state: "", scope: " ", slug: "" },
58+
schema,
59+
keepNull,
60+
),
4861
).toEqual({});
4962
});
5063

5164
test("forwards an empty string when the pattern does not compile", () => {
52-
expect(stripPlaceholderArguments({ malformed: "" }, schema)).toEqual({
65+
expect(
66+
stripPlaceholderArguments({ malformed: "" }, schema, keepNull),
67+
).toEqual({
5368
malformed: "",
5469
});
5570
});
5671

5772
test("drops an empty array only where minItems is at least 1", () => {
5873
expect(
59-
stripPlaceholderArguments({ fields: [], labels: [] }, schema),
74+
stripPlaceholderArguments({ fields: [], labels: [] }, schema, keepNull),
6075
).toEqual({ fields: [] });
6176
});
6277

@@ -65,48 +80,61 @@ describe("stripPlaceholderArguments", () => {
6580
stripPlaceholderArguments(
6681
{ priority: "", includeArchived: " ", query: [] },
6782
schema,
83+
keepNull,
6884
),
6985
).toEqual({});
7086
});
7187

7288
test("drops null only where the property schema disallows it", () => {
7389
expect(
74-
stripPlaceholderArguments({ team: null, priority: null }, schema),
90+
stripPlaceholderArguments(
91+
{ team: null, priority: null },
92+
schema,
93+
keepNull,
94+
),
7595
).toEqual({});
7696
const nullable = {
7797
assignee: null,
7898
parentId: null,
7999
cycle: null,
80100
anything: null,
81101
};
82-
expect(stripPlaceholderArguments(nullable, schema)).toEqual(nullable);
102+
expect(stripPlaceholderArguments(nullable, schema, keepNull)).toEqual(
103+
nullable,
104+
);
83105
});
84106

85107
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({
108+
expect(stripPlaceholderArguments({ view: " " }, schema, keepNull)).toEqual(
109+
{},
110+
);
111+
expect(stripPlaceholderArguments({ view: null }, schema, keepNull)).toEqual(
112+
{
113+
view: null,
114+
},
115+
);
116+
expect(stripPlaceholderArguments({ label: "" }, schema, keepNull)).toEqual({
91117
label: "",
92118
});
93119
});
94120

95121
test("never changes required arguments", () => {
96-
expect(stripPlaceholderArguments({ id: "" }, schema)).toEqual({ id: "" });
97-
expect(stripPlaceholderArguments({ id: null }, schema)).toEqual({
122+
expect(stripPlaceholderArguments({ id: "" }, schema, keepNull)).toEqual({
123+
id: "",
124+
});
125+
expect(stripPlaceholderArguments({ id: null }, schema, keepNull)).toEqual({
98126
id: null,
99127
});
100128
});
101129

102130
test("keeps zero and false", () => {
103131
const args = { priority: 0, includeArchived: false };
104-
expect(stripPlaceholderArguments(args, schema)).toEqual(args);
132+
expect(stripPlaceholderArguments(args, schema, keepNull)).toEqual(args);
105133
});
106134

107135
test("keeps meaningful values and leaves the caller's object intact", () => {
108136
const args = { query: "release", fields: ["title"], customView: "" };
109-
expect(stripPlaceholderArguments(args, schema)).toEqual({
137+
expect(stripPlaceholderArguments(args, schema, keepNull)).toEqual({
110138
query: "release",
111139
fields: ["title"],
112140
});
@@ -119,22 +147,69 @@ describe("stripPlaceholderArguments", () => {
119147

120148
test("forwards a key the schema does not declare", () => {
121149
expect(
122-
stripPlaceholderArguments({ id: "CL-1", extra: "" }, schema),
150+
stripPlaceholderArguments({ id: "CL-1", extra: "" }, schema, keepNull),
123151
).toEqual({ id: "CL-1", extra: "" });
124152
});
125153

126154
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");
155+
expect(stripPlaceholderArguments(null, schema, keepNull)).toBeNull();
156+
expect(
157+
stripPlaceholderArguments(undefined, schema, keepNull),
158+
).toBeUndefined();
159+
expect(stripPlaceholderArguments("query", schema, keepNull)).toBe("query");
130160
const list = [""];
131-
expect(stripPlaceholderArguments(list, schema)).toBe(list);
161+
expect(stripPlaceholderArguments(list, schema, keepNull)).toBe(list);
132162
});
133163

134164
test("forwards arguments unchanged when the schema shape is unusable", () => {
135165
const args = { query: "" };
136166
expect(
137-
stripPlaceholderArguments(args, { type: "object", required: "query" }),
167+
stripPlaceholderArguments(
168+
args,
169+
{ type: "object", required: "query" },
170+
keepNull,
171+
),
138172
).toEqual(args);
139173
});
140174
});
175+
176+
describe("stripPlaceholderArguments with nullMeansUnset", () => {
177+
test("drops null on optional properties whose schema permits it", () => {
178+
expect(
179+
stripPlaceholderArguments(
180+
{
181+
assignee: null,
182+
parentId: null,
183+
cycle: null,
184+
view: null,
185+
anything: null,
186+
query: "release",
187+
},
188+
schema,
189+
dropNull,
190+
),
191+
).toEqual({ query: "release" });
192+
});
193+
194+
test("still applies the rejected-placeholder rule to other values", () => {
195+
expect(
196+
stripPlaceholderArguments(
197+
{ customView: " ", fields: [], priority: 0, team: null },
198+
schema,
199+
dropNull,
200+
),
201+
).toEqual({ fields: [], priority: 0 });
202+
});
203+
204+
test("never changes required arguments", () => {
205+
expect(stripPlaceholderArguments({ id: null }, schema, dropNull)).toEqual({
206+
id: null,
207+
});
208+
});
209+
210+
test("forwards null on a key the schema does not declare", () => {
211+
expect(
212+
stripPlaceholderArguments({ extra: null }, schema, dropNull),
213+
).toEqual({ extra: null });
214+
});
215+
});

‎src/mcp/placeholder-args.ts‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,35 +72,56 @@ function isRejectedPlaceholder(value: unknown, property: unknown): boolean {
7272
return value === null && !admits(property, null);
7373
}
7474

75+
export interface StripPlaceholderOpts {
76+
// A read tool's server takes a `null` filter literally (Linear's
77+
// list_issues reads `assignee: null` as "unassigned"), so padding narrows
78+
// the results; a write tool's `null` clears the field and must survive.
79+
// The cost: a deliberate JSON-null filter on a read tool is dropped for
80+
// every model, so asking for an unset field takes the string form a server
81+
// documents (Linear's `"null"`).
82+
nullMeansUnset: boolean;
83+
}
84+
7585
/**
7686
* Drops optional arguments a model filled with an empty placeholder (`""`,
7787
* 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.
88+
* so one padded field does not fail the whole call. With `nullMeansUnset`,
89+
* `null` on any declared optional property is dropped even where the schema
90+
* permits it. Other empty values the schema permits, required arguments,
91+
* undeclared keys, and any other value are forwarded unchanged.
8092
*/
8193
export function stripPlaceholderArguments(
8294
args: Record<string, unknown>,
8395
inputSchema: Record<string, unknown>,
96+
opts: StripPlaceholderOpts,
8497
): Record<string, unknown>;
8598
export function stripPlaceholderArguments(
8699
args: unknown,
87100
inputSchema: Record<string, unknown>,
101+
opts: StripPlaceholderOpts,
88102
): unknown;
89103
export function stripPlaceholderArguments(
90104
args: unknown,
91105
inputSchema: Record<string, unknown>,
106+
opts: StripPlaceholderOpts,
92107
): unknown {
93108
const parsedArgs = Arguments(args);
94109
if (parsedArgs instanceof type.errors || Array.isArray(parsedArgs))
95110
return args;
96111
const schema = ObjectSchema(inputSchema);
97112
if (schema instanceof type.errors) return args;
98113
const required = new Set(schema.required ?? []);
114+
const properties = schema.properties ?? {};
99115
return Object.fromEntries(
100-
Object.entries(parsedArgs).filter(
101-
([key, value]) =>
102-
required.has(key) ||
103-
!isRejectedPlaceholder(value, schema.properties?.[key]),
104-
),
116+
Object.entries(parsedArgs).filter(([key, value]) => {
117+
if (required.has(key)) return true;
118+
if (
119+
opts.nullMeansUnset &&
120+
value === null &&
121+
Object.hasOwn(properties, key)
122+
)
123+
return false;
124+
return !isRejectedPlaceholder(value, properties[key]);
125+
}),
105126
);
106127
}

‎src/mcp/plugin.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -734,6 +734,63 @@ describe("mcpClientToAgentTools (production gated path)", () => {
734734
expect(result.isError).toBeUndefined();
735735
});
736736

737+
test.each([
738+
{ name: "list_issues", annotations: undefined, forwarded: {} },
739+
{
740+
name: "save_issue",
741+
annotations: undefined,
742+
forwarded: { assignee: null },
743+
},
744+
{ name: "lookup", annotations: { readOnlyHint: true }, forwarded: {} },
745+
{
746+
name: "list_issues",
747+
annotations: { readOnlyHint: false },
748+
forwarded: { assignee: null },
749+
},
750+
])(
751+
"$name with annotations $annotations forwards $forwarded for an optional null",
752+
async ({ name, annotations, forwarded }) => {
753+
let capturedArgs: Record<string, unknown> | undefined;
754+
const client: MCPClient = {
755+
serverName: "linear",
756+
tools: [
757+
{
758+
name,
759+
description: "issues",
760+
inputSchema: {
761+
type: "object",
762+
properties: { assignee: { type: ["string", "null"] } },
763+
},
764+
...(annotations === undefined ? {} : { annotations }),
765+
},
766+
],
767+
async callResult(_toolName, args) {
768+
capturedArgs = args;
769+
return { blocks: [{ type: "text", text: "done" }], isError: false };
770+
},
771+
async close() {
772+
return undefined;
773+
},
774+
};
775+
const gate = createPermissionGate({
776+
approvals: [],
777+
interactive: false,
778+
skipPermissions: true,
779+
reactorGated: false,
780+
});
781+
const tool = defined(mcpClientToAgentTools(client, gate)[0], "mcp tool");
782+
await tool.handler(
783+
{
784+
id: "c1",
785+
name: `mcp__linear__${name}`,
786+
arguments: { assignee: null },
787+
},
788+
new AbortController().signal,
789+
);
790+
expect(capturedArgs).toEqual(forwarded);
791+
},
792+
);
793+
737794
test("permission gate blocks mutating MCP when not skipped", async () => {
738795
let asked = 0;
739796
const gate = createPermissionGate({

‎src/mcp/plugin.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
} from "./client.js";
2121
import { stripPlaceholderArguments } from "./placeholder-args.js";
2222
import { mcpToolName } from "./tool-name.js";
23+
import { isReadOnlyMcpToolDefinition } from "./tool-permissions.js";
2324
import { LOG_NAMESPACE_ROOT } from "../branding.js";
2425

2526
export const MCP_RECONNECTING_TOOL_ERROR =
@@ -108,11 +109,18 @@ export function mcpClientTools(
108109
signal: AbortSignal,
109110
): Promise<ToolResult> => {
110111
try {
111-
// Some models pad every optional field with empty values; one the
112-
// property schema rejects (customView minLength) fails the call.
112+
// Some models pad every optional field with empty values: one the
113+
// property schema rejects (customView minLength) fails the call,
114+
// and a null on a read tool silently filters the results.
113115
const envelope = await client.callResult(
114116
tool.name,
115-
stripPlaceholderArguments(call.arguments, tool.inputSchema),
117+
stripPlaceholderArguments(call.arguments, tool.inputSchema, {
118+
nullMeansUnset: isReadOnlyMcpToolDefinition(
119+
tool.annotations,
120+
client.serverName,
121+
tool.name,
122+
),
123+
}),
116124
signal,
117125
);
118126
const authorizedBlocks = applyPolicyToBlocks(envelope.blocks);

0 commit comments

Comments
 (0)