From f3126c34ef7053bcd03b2d94c32640d0f5344c19 Mon Sep 17 00:00:00 2001 From: Ho Lim <166576253+HOYALIM@users.noreply.github.com> Date: Tue, 21 Jul 2026 00:28:22 -0700 Subject: [PATCH] fix(gateway): reduce mcp loopback schema warning noise (#103171) * fix(gateway): reduce mcp loopback schema warning noise Signed-off-by: Ho Lim * test(gateway): prove mcp warning behavior over loopback HTTP Signed-off-by: Ho Lim * test(gateway): update mcp loopback warning prefix --------- Signed-off-by: Ho Lim Co-authored-by: Peter Steinberger --- src/gateway/mcp-http.schema.ts | 62 +++++++++++++++++- src/gateway/mcp-http.test.ts | 99 +++++++++++++++++++++++++++-- src/gateway/mcp-http.ts | 2 +- src/logging/console-capture.test.ts | 17 +++++ 4 files changed, 170 insertions(+), 10 deletions(-) diff --git a/src/gateway/mcp-http.schema.ts b/src/gateway/mcp-http.schema.ts index 7dee7f580729..12e212da7746 100644 --- a/src/gateway/mcp-http.schema.ts +++ b/src/gateway/mcp-http.schema.ts @@ -5,6 +5,8 @@ import { isRecord } from "@openclaw/normalization-core/record-coerce"; import { logWarn } from "../logger.js"; import { resolveGatewayScopedTools } from "./tool-resolution.js"; +const MCP_LOOPBACK_LOG_PREFIX = "mcp-loopback"; + // MCP loopback schema projection adapts gateway tool definitions into MCP // tools/list entries. It flattens provider-hostile union schemas into object // schemas because some MCP clients cannot render anyOf/oneOf controls. @@ -145,7 +147,7 @@ function flattenUnionSchema( for (const [key, schema] of Object.entries(props)) { if (!isPropertySchema(schema)) { warnSchemaOnce( - `mcp loopback: malformed schema definition for "${toolName}.${key}", ignoring that variant`, + `${MCP_LOOPBACK_LOG_PREFIX}: malformed schema definition for "${toolName}.${key}", ignoring that variant`, ); continue; } @@ -166,10 +168,13 @@ function flattenUnionSchema( if (incoming === false) { continue; } + if (areSchemaValuesEquivalent(existing, incoming)) { + continue; + } if (!isRecord(existing) || !isRecord(incoming)) { if (existing !== incoming) { warnSchemaOnce( - `mcp loopback: conflicting schema definitions for "${toolName}.${key}", keeping the first variant`, + `${MCP_LOOPBACK_LOG_PREFIX}: conflicting schema definitions for "${toolName}.${key}", keeping the first variant`, ); } continue; @@ -185,7 +190,7 @@ function flattenUnionSchema( continue; } warnSchemaOnce( - `mcp loopback: conflicting schema definitions for "${toolName}.${key}", keeping the first variant`, + `${MCP_LOOPBACK_LOG_PREFIX}: conflicting schema definitions for "${toolName}.${key}", keeping the first variant`, ); } } @@ -207,6 +212,57 @@ function isPropertySchema(value: unknown): value is boolean | Record>, +): boolean { + const existing = seen.get(left); + if (existing?.has(right)) { + return true; + } + const next = existing ?? new WeakSet(); + next.add(right); + if (!existing) { + seen.set(left, next); + } + return false; +} + +function areSchemaValuesEquivalent( + left: unknown, + right: unknown, + seen = new WeakMap>(), +): boolean { + if (Object.is(left, right)) { + return true; + } + if (Array.isArray(left) || Array.isArray(right)) { + if (!Array.isArray(left) || !Array.isArray(right) || left.length !== right.length) { + return false; + } + if (rememberSchemaPair(left, right, seen)) { + return true; + } + return left.every((value, index) => areSchemaValuesEquivalent(value, right[index], seen)); + } + if (!isRecord(left) || !isRecord(right)) { + return false; + } + if (rememberSchemaPair(left, right, seen)) { + return true; + } + const leftKeys = Object.keys(left).toSorted(); + const rightKeys = Object.keys(right).toSorted(); + if (leftKeys.length !== rightKeys.length) { + return false; + } + return leftKeys.every( + (key, index) => + key === rightKeys[index] && areSchemaValuesEquivalent(left[key], right[key], seen), + ); +} + // Loopback schemas are rebuilt on every cache miss (per session/owner context and // after TTL expiry), so raw logWarn would repeat the same field warning endlessly. // Dedupe on the full message: distinct (tool, field, reason) still each warn once, diff --git a/src/gateway/mcp-http.test.ts b/src/gateway/mcp-http.test.ts index b534b2a90086..4d18d2b4bbd3 100644 --- a/src/gateway/mcp-http.test.ts +++ b/src/gateway/mcp-http.test.ts @@ -863,7 +863,7 @@ describe("buildMcpToolSchema", () => { }, }); expect(logWarnMock.mock.calls.map(([message]) => message)).toEqual([ - 'mcp loopback: conflicting schema definitions for "codex_threads_constrained_literals.action", keeping the first variant', + 'mcp-loopback: conflicting schema definitions for "codex_threads_constrained_literals.action", keeping the first variant', ]); }); @@ -930,11 +930,53 @@ describe("buildMcpToolSchema", () => { } expect(logWarnMock.mock.calls.map(([message]) => message)).toEqual([ - 'mcp loopback: conflicting schema definitions for "mcp_message_send_rebuild.action", keeping the first variant', - 'mcp loopback: conflicting schema definitions for "mcp_message_send_rebuild.callId", keeping the first variant', + 'mcp-loopback: conflicting schema definitions for "mcp_message_send_rebuild.action", keeping the first variant', + 'mcp-loopback: conflicting schema definitions for "mcp_message_send_rebuild.callId", keeping the first variant', ]); }); + it("does not warn for structurally identical union property schemas", () => { + const tool = makeMockTool({ + name: "lark_doc_read", + parameters: { + anyOf: [ + { + type: "object", + properties: { + doc_token: { + type: "string", + description: "Lark document token", + minLength: 1, + }, + }, + }, + { + type: "object", + properties: { + doc_token: { + minLength: 1, + description: "Lark document token", + type: "string", + }, + }, + }, + ], + }, + }); + + expect(buildMockMcpToolSchema([tool])[0]?.inputSchema).toMatchObject({ + type: "object", + properties: { + doc_token: { + type: "string", + description: "Lark document token", + minLength: 1, + }, + }, + }); + expect(logWarnMock).not.toHaveBeenCalled(); + }); + it("warns per tool for the same conflicting field name across different tools", () => { const conflictingUnion = (label: string) => ({ anyOf: [ @@ -956,8 +998,8 @@ describe("buildMcpToolSchema", () => { buildMockMcpToolSchema([messageTool, calendarTool]); expect(logWarnMock.mock.calls.map(([message]) => message)).toEqual([ - 'mcp loopback: conflicting schema definitions for "mcp_message_send_per_tool.action", keeping the first variant', - 'mcp loopback: conflicting schema definitions for "mcp_calendar_create_per_tool.action", keeping the first variant', + 'mcp-loopback: conflicting schema definitions for "mcp_message_send_per_tool.action", keeping the first variant', + 'mcp-loopback: conflicting schema definitions for "mcp_calendar_create_per_tool.action", keeping the first variant', ]); }); @@ -976,12 +1018,57 @@ describe("buildMcpToolSchema", () => { buildMockMcpToolSchema([tool]); expect(logWarnMock.mock.calls.map(([message]) => message)).toEqual([ - 'mcp loopback: malformed schema definition for "mcp_message_send_malformed.action", ignoring that variant', + 'mcp-loopback: malformed schema definition for "mcp_message_send_malformed.action", ignoring that variant', ]); }); }); describe("mcp loopback server", () => { + it("keeps equal schemas quiet and dedupes genuine conflicts across HTTP cache misses", async () => { + mockScopedTools([ + makeMockTool({ + name: "lark_doc_read", + parameters: { + anyOf: [ + { + type: "object", + properties: { + doc_token: { type: "string", description: "Lark document token" }, + action: { type: "string" }, + }, + }, + { + type: "object", + properties: { + doc_token: { description: "Lark document token", type: "string" }, + action: { type: "number" }, + }, + }, + ], + }, + }), + ]); + const { runtime } = await startLoopbackServerForTest(); + + for (let index = 0; index < 3; index += 1) { + const payload = await readOkMcpPayload( + await sendLoopbackToolsList({ + token: runtime.ownerToken, + headers: { + ...MAIN_SESSION_HEADER, + "x-openclaw-current-message-id": `message-${index}`, + }, + }), + ); + expectMcpToolNames(payload, ["lark_doc_read"]); + } + + expect(resolveGatewayScopedToolsMock).toHaveBeenCalledTimes(3); + expect(logWarnMock.mock.calls.map(([message]) => message)).toEqual([ + 'mcp-loopback: conflicting schema definitions for "lark_doc_read.action", keeping the first variant', + ]); + }); + it("rejects reserved harness contexts before tool resolution", async () => { const { runtime } = await startLoopbackServerForTest(); const response = await sendLoopbackToolsList({ diff --git a/src/gateway/mcp-http.ts b/src/gateway/mcp-http.ts index a37eee646a7a..df50612a7fea 100644 --- a/src/gateway/mcp-http.ts +++ b/src/gateway/mcp-http.ts @@ -418,7 +418,7 @@ async function startMcpLoopbackServer(port = 0): Promise<{ res.writeHead(200, { "Content-Type": "application/json" }); res.end(payload); } catch (error) { - logWarn(`mcp loopback: request handling failed: ${formatErrorMessage(error)}`); + logWarn(`mcp-loopback: request handling failed: ${formatErrorMessage(error)}`); logMcpLoopbackTraffic("request-failed", { message: formatErrorMessage(error), }); diff --git a/src/logging/console-capture.test.ts b/src/logging/console-capture.test.ts index bb300806ff91..9c8e3b0d6405 100644 --- a/src/logging/console-capture.test.ts +++ b/src/logging/console-capture.test.ts @@ -1,6 +1,8 @@ // Console capture tests cover intercepting and restoring console output. +import fs from "node:fs"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { setVerbose } from "../global-state.js"; +import { logWarn } from "../logger.js"; import { enableConsoleCapture, resetLogger, @@ -137,6 +139,17 @@ describe("enableConsoleCapture", () => { expect(stdoutWrite).toHaveBeenCalledWith('{\n "ok": true\n}\n'); }); + it("routes subsystem-prefixed warnings through one file-log sink", () => { + const logPath = tempLogPath(); + setLoggerOverride({ level: "info", file: logPath }); + enableConsoleCapture(); + + logWarn("mcp-loopback: conflicting schema definitions"); + + const content = fs.readFileSync(logPath, "utf-8"); + expect(countMatchingLines(content, "conflicting schema definitions")).toBe(1); + }); + it("redacts credentials before forwarding console output", () => { setLoggerOverride({ level: "info", file: tempLogPath() }); const log = vi.fn(); @@ -245,6 +258,10 @@ function tempLogPath() { return logPathTracker.nextPath(); } +function countMatchingLines(value: string, needle: string): number { + return value.split(/\r?\n/u).filter((line) => line.includes(needle)).length; +} + function eioError() { const err = new Error("EIO") as NodeJS.ErrnoException; err.code = "EIO";