mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix(gateway): suppress MCP notification responses (#101730)
* fix(gateway): suppress mcp notification responses * fix(gateway): MCP clients receive responses for JSON-RPC notifications * Handle MCP lifecycle notifications before tool resolution * Filter notification entries from internal-error batches * fix(gateway): centralize MCP response eligibility Co-authored-by: 张贵萍0668001030 <zhang.guiping@xydigit.com> * test(gateway): use public MCP loopback helper Co-authored-by: 张贵萍0668001030 <zhang.guiping@xydigit.com> --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
co-authored by
Peter Steinberger
parent
0497148458
commit
c7f58f5bed
@@ -998,6 +998,40 @@ describe("mcp loopback server", () => {
|
||||
expect(resolveGatewayScopedToolsMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("suppresses reserved-harness errors for notifications", async () => {
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const headers = {
|
||||
"content-type": "application/json",
|
||||
"x-session-key": "agent:main:harness:codex:supervision:native-thread",
|
||||
};
|
||||
const notificationResponse = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers,
|
||||
body: JSON.stringify({ jsonrpc: "2.0", method: "tools/list" }),
|
||||
});
|
||||
const mixedResponse = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers,
|
||||
body: JSON.stringify([
|
||||
{ jsonrpc: "2.0", method: "tools/list" },
|
||||
{ jsonrpc: "2.0", id: 42, method: "tools/list" },
|
||||
]),
|
||||
});
|
||||
|
||||
expect(notificationResponse.status).toBe(202);
|
||||
await expect(notificationResponse.text()).resolves.toBe("");
|
||||
expect(mixedResponse.status).toBe(200);
|
||||
await expect(mixedResponse.json()).resolves.toEqual([
|
||||
expect.objectContaining({
|
||||
id: 42,
|
||||
error: { code: -32600, message: expect.stringContaining("reserved") },
|
||||
}),
|
||||
]);
|
||||
expect(resolveGatewayScopedToolsMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows an existing unlocked legacy harness-prefixed context", async () => {
|
||||
const { runtime } = await startLoopbackServerForTest();
|
||||
const sessionKey = "agent:main:harness:legacy-notes";
|
||||
@@ -2843,6 +2877,36 @@ describe("mcp loopback server", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("does not include notifications in internal-error batch responses", async () => {
|
||||
resolveGatewayScopedToolsMock.mockImplementation(() => {
|
||||
throw new Error("tool resolution exploded");
|
||||
});
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify([
|
||||
{ jsonrpc: "2.0", method: "tools/list" },
|
||||
{ jsonrpc: "2.0", id: 42, method: "tools/list" },
|
||||
]),
|
||||
});
|
||||
const payload = (await response.json()) as Array<{
|
||||
id?: unknown;
|
||||
error?: { code?: number; message?: string };
|
||||
}>;
|
||||
|
||||
expect(response.status).toBe(500);
|
||||
expect(payload).toHaveLength(1);
|
||||
expect(payload[0]).toMatchObject({
|
||||
id: 42,
|
||||
error: {
|
||||
code: -32603,
|
||||
message: "Internal error",
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("returns invalid request errors for malformed batch entries without resetting the request", async () => {
|
||||
server = await ensureMcpLoopbackServer(0);
|
||||
const runtime = getActiveMcpLoopbackRuntime();
|
||||
@@ -2871,6 +2935,97 @@ describe("mcp loopback server", () => {
|
||||
expect(payload[1]?.result?.tools?.map((tool) => tool.name)).toContain("message");
|
||||
});
|
||||
|
||||
it("returns an invalid request for an empty batch before harness policy", async () => {
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: {
|
||||
"content-type": "application/json",
|
||||
"x-session-key": "agent:main:harness:codex:supervision:native-thread",
|
||||
},
|
||||
body: "[]",
|
||||
});
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
await expect(response.json()).resolves.toMatchObject({
|
||||
id: null,
|
||||
error: { code: -32600, message: "Invalid Request" },
|
||||
});
|
||||
expect(resolveGatewayScopedToolsMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns 202 with an empty body for JSON-RPC notifications", async () => {
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ jsonrpc: "2.0", method: "tools/list" }),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(202);
|
||||
await expect(response.text()).resolves.toBe("");
|
||||
});
|
||||
|
||||
it("suppresses internal errors for notification-only requests", async () => {
|
||||
resolveGatewayScopedToolsMock.mockImplementation(() => {
|
||||
throw new Error("tool resolution exploded");
|
||||
});
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ jsonrpc: "2.0", method: "tools/list" }),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(202);
|
||||
await expect(response.text()).resolves.toBe("");
|
||||
expect(resolveGatewayScopedToolsMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("returns only request responses for mixed batches", async () => {
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify([
|
||||
{ jsonrpc: "2.0", method: "tools/list" },
|
||||
{ jsonrpc: "2.0", id: 42, method: "tools/list" },
|
||||
]),
|
||||
});
|
||||
const payload = (await response.json()) as Array<{ id?: unknown }>;
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(payload).toHaveLength(1);
|
||||
expect(payload[0]?.id).toBe(42);
|
||||
});
|
||||
|
||||
it("dispatches tools/call notifications without returning a response", async () => {
|
||||
const execute = vi.fn(async () => ({
|
||||
content: [{ type: "text", text: "ok" }],
|
||||
}));
|
||||
mockScopedTools([makeMessageTool({ execute })]);
|
||||
const { runtime, port } = await startLoopbackServerForTest();
|
||||
const response = await sendRaw({
|
||||
port,
|
||||
token: runtime.ownerToken,
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
jsonrpc: "2.0",
|
||||
method: "tools/call",
|
||||
params: { name: "message", arguments: { body: "hello" } },
|
||||
}),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(202);
|
||||
await expect(response.text()).resolves.toBe("");
|
||||
expect(resolveGatewayScopedToolsMock).toHaveBeenCalledTimes(1);
|
||||
expect(execute).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("returns 413 instead of resetting oversized request bodies", async () => {
|
||||
server = await ensureMcpLoopbackServer(0);
|
||||
const runtime = getActiveMcpLoopbackRuntime();
|
||||
|
||||
+44
-13
@@ -74,9 +74,9 @@ function isMcpJsonParseError(error: unknown): error is Error & { code: "mcp_json
|
||||
);
|
||||
}
|
||||
|
||||
function parseMcpJsonBody(body: string): JsonRpcRequest | JsonRpcRequest[] {
|
||||
function parseMcpJsonBody(body: string): unknown {
|
||||
try {
|
||||
return JSON.parse(body) as JsonRpcRequest | JsonRpcRequest[];
|
||||
return JSON.parse(body) as unknown;
|
||||
} catch (error) {
|
||||
throw createMcpJsonParseError(error);
|
||||
}
|
||||
@@ -94,13 +94,27 @@ function isJsonRpcRequest(message: unknown): message is JsonRpcRequest {
|
||||
return isRecord(message) && message.jsonrpc === "2.0" && typeof message.method === "string";
|
||||
}
|
||||
|
||||
function jsonRpcInternalError(parsed: JsonRpcRequest | JsonRpcRequest[] | undefined) {
|
||||
if (Array.isArray(parsed)) {
|
||||
return parsed.map((message) =>
|
||||
jsonRpcError(readJsonRpcRequestId(message), -32603, "Internal error"),
|
||||
);
|
||||
function shouldSendJsonRpcResponse(message: unknown): boolean {
|
||||
return !isJsonRpcRequest(message) || Object.hasOwn(message, "id");
|
||||
}
|
||||
|
||||
function collectJsonRpcResponses<T>(
|
||||
messages: unknown[],
|
||||
createResponse: (message: unknown) => T,
|
||||
): T[] {
|
||||
return messages.filter(shouldSendJsonRpcResponse).map(createResponse);
|
||||
}
|
||||
|
||||
function jsonRpcInternalError(parsed: unknown) {
|
||||
const isBatch = Array.isArray(parsed);
|
||||
const messages = isBatch ? parsed : [parsed];
|
||||
const responses = collectJsonRpcResponses(messages, (message) =>
|
||||
jsonRpcError(readJsonRpcRequestId(message), -32603, "Internal error"),
|
||||
);
|
||||
if (responses.length === 0) {
|
||||
return null;
|
||||
}
|
||||
return jsonRpcError(readJsonRpcRequestId(parsed), -32603, "Internal error");
|
||||
return isBatch ? responses : responses[0];
|
||||
}
|
||||
|
||||
function shouldLogMcpLoopbackTraffic(): boolean {
|
||||
@@ -201,11 +215,17 @@ async function startMcpLoopbackServer(port = 0): Promise<{
|
||||
const cliRequestCaptureHandle = markMcpLoopbackRequestStarted(cliCaptureKey);
|
||||
const requestAbort = createRequestAbortSignal(req, res);
|
||||
void (async () => {
|
||||
let parsed: JsonRpcRequest | JsonRpcRequest[] | undefined;
|
||||
let parsed: unknown;
|
||||
let cliCaptureHandles: Array<ReturnType<typeof markMcpLoopbackToolCallStarted>> = [];
|
||||
try {
|
||||
const body = await readMcpHttpBody(req, { timeoutMs: resolveMcpHttpBodyTimeoutMs() });
|
||||
parsed = parseMcpJsonBody(body);
|
||||
if (Array.isArray(parsed) && parsed.length === 0) {
|
||||
markMcpLoopbackRequestClassified(cliRequestCaptureHandle);
|
||||
res.writeHead(200, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify(jsonRpcError(null, -32600, "Invalid Request")));
|
||||
return;
|
||||
}
|
||||
const messages = Array.isArray(parsed) ? parsed : [parsed];
|
||||
cliCaptureHandles = messages.map((message) => {
|
||||
if (
|
||||
@@ -252,13 +272,18 @@ async function startMcpLoopbackServer(port = 0): Promise<{
|
||||
(!harnessEntry ||
|
||||
isAgentHarnessSessionStoreEntryProtected(requestContext.sessionKey, harnessEntry))
|
||||
) {
|
||||
const errors = messages.map((message) =>
|
||||
const errors = collectJsonRpcResponses(messages, (message) =>
|
||||
jsonRpcError(
|
||||
readJsonRpcRequestId(message),
|
||||
-32600,
|
||||
AGENT_HARNESS_SESSION_KEY_RESERVED_MESSAGE,
|
||||
),
|
||||
);
|
||||
if (errors.length === 0) {
|
||||
res.writeHead(202);
|
||||
res.end();
|
||||
return;
|
||||
}
|
||||
const payload = Array.isArray(parsed)
|
||||
? JSON.stringify(errors)
|
||||
: JSON.stringify(errors[0]);
|
||||
@@ -368,7 +393,7 @@ async function startMcpLoopbackServer(port = 0): Promise<{
|
||||
} finally {
|
||||
markMcpLoopbackToolCallFinished(cliCaptureHandle);
|
||||
}
|
||||
if (response !== null) {
|
||||
if (response !== null && shouldSendJsonRpcResponse(message)) {
|
||||
const responseToolName =
|
||||
message.method === "tools/call" && isRecord(message.params)
|
||||
? message.params.name
|
||||
@@ -415,8 +440,14 @@ async function startMcpLoopbackServer(port = 0): Promise<{
|
||||
res.writeHead(400, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify(jsonRpcError(null, -32700, "Parse error")));
|
||||
} else {
|
||||
res.writeHead(500, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify(jsonRpcInternalError(parsed)));
|
||||
const internalError = jsonRpcInternalError(parsed);
|
||||
if (internalError === null) {
|
||||
res.writeHead(202);
|
||||
res.end();
|
||||
} else {
|
||||
res.writeHead(500, { "Content-Type": "application/json" });
|
||||
res.end(JSON.stringify(internalError));
|
||||
}
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
|
||||
Reference in New Issue
Block a user