fix(mcp-service): sanitize MCP call arguments to avoid 'unhashable type: dict' from MCP servers#14886
fix(mcp-service): sanitize MCP call arguments to avoid 'unhashable type: dict' from MCP servers#14886404-Page-Found wants to merge 1 commit into
Conversation
… causing Python server errors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: 404-Page-Found <Lucas20220605@gmail.com>
|
add a unit test plz |
|
Note This issue/comment/review was translated by Claude. Thanks for the fix! However, I have a few points to discuss with you during review of this change. It may be worth confirming the root cause and implementation approach again before merging. 1. About the Root Cause
This means the args reaching the Python server must be pure JSON—Map/Set become A more likely cause is: the model gave a nested object as arg (e.g., schema expects string, but model gave If convenient, could you reproduce once with the 2. If Keeping Sanitize, Types Can Be SaferThe current signature is type JsonPrimitive = string | number | boolean | null
type JsonValue = JsonPrimitive | JsonValue[] | { [k: string]: JsonValue }
function sanitizeForMcp(val: unknown, seen: WeakSet<object> = new WeakSet()): JsonValue {
...
}
3. Several Edge Cases That Will ErrorThe current implementation uses
Reference implementation: function sanitizeForMcp(val: unknown, seen: WeakSet<object> = new WeakSet()): JsonValue {
if (val === null || val === undefined) return null
switch (typeof val) {
case 'string':
case 'boolean': return val
case 'number': return Number.isFinite(val) ? val : null
case 'bigint': return val.toString()
case 'function':
case 'symbol': return null
}
if (seen.has(val as object)) return null
seen.add(val as object)
if (Array.isArray(val)) return val.map((v) => sanitizeForMcp(v, seen))
if (val instanceof Date) return val.toISOString()
if (val instanceof Map) {
const out: Record<string, JsonValue> = {}
for (const [k, v] of val) out[String(k)] = sanitizeForMcp(v, seen)
return out
}
if (val instanceof Set) return Array.from(val, (v) => sanitizeForMcp(v, seen))
if (val instanceof Error) return { name: val.name, message: val.message }
if (val instanceof URL || val instanceof RegExp) return String(val)
if (ArrayBuffer.isView(val) || val instanceof ArrayBuffer) {
return Buffer.from(val as ArrayBufferLike).toString('base64')
}
if (typeof (val as { toJSON?: unknown }).toJSON === 'function') {
return sanitizeForMcp((val as { toJSON: () => unknown }).toJSON(), seen)
}
const out: Record<string, JsonValue> = {}
for (const [k, v] of Object.entries(val as object)) {
out[k] = sanitizeForMcp(v, seen)
}
return out
}At the call site, use const sanitized = sanitizeForMcp(args) as Record<string, unknown>
const result = await client.callTool({ name, arguments: sanitized }, ...)4. The Outer
|
| 输入 | 当前结果 | 期望 |
|---|---|---|
Date |
{} |
date.toISOString() |
Buffer / Uint8Array |
{ "0": 72, "1": 105, ... } |
base64 |
Error |
{} |
{ name, message } |
URL / RegExp |
{} |
String(val) |
带 toJSON() 的对象 |
丢 toJSON 走原型枚举 |
先调 toJSON() |
| 循环引用 | 栈溢出 | WeakSet 检测 |
参考实现：
function sanitizeForMcp(val: unknown, seen: WeakSet<object> = new WeakSet()): JsonValue {
if (val === null || val === undefined) return null
switch (typeof val) {
case 'string':
case 'boolean': return val
case 'number': return Number.isFinite(val) ? val : null
case 'bigint': return val.toString()
case 'function':
case 'symbol': return null
}
if (seen.has(val as object)) return null
seen.add(val as object)
if (Array.isArray(val)) return val.map((v) => sanitizeForMcp(v, seen))
if (val instanceof Date) return val.toISOString()
if (val instanceof Map) {
const out: Record<string, JsonValue> = {}
for (const [k, v] of val) out[String(k)] = sanitizeForMcp(v, seen)
return out
}
if (val instanceof Set) return Array.from(val, (v) => sanitizeForMcp(v, seen))
if (val instanceof Error) return { name: val.name, message: val.message }
if (val instanceof URL || val instanceof RegExp) return String(val)
if (ArrayBuffer.isView(val) || val instanceof ArrayBuffer) {
return Buffer.from(val as ArrayBufferLike).toString('base64')
}
if (typeof (val as { toJSON?: unknown }).toJSON === 'function') {
return sanitizeForMcp((val as { toJSON: () => unknown }).toJSON(), seen)
}
const out: Record<string, JsonValue> = {}
for (const [k, v] of Object.entries(val as object)) {
out[k] = sanitizeForMcp(v, seen)
}
return out
}调用处用 as Record<string, unknown> 对齐 SDK：
const sanitized = sanitizeForMcp(args) as Record<string, unknown>
const result = await client.callTool({ name, arguments: sanitized }, ...)4. 外层的 try/catch 可以删掉
try {
args = sanitizeForMcp(args)
} catch (e) {
...warn('Failed to sanitize args, sending raw', e)
}上面这个函数对纯 JSON 输入不会抛——BigInt/cycle 都在内部 handle 了。保留 catch 反而误导 reviewer 觉得这里有未知风险。
总结一下我的建议优先级：
- 先用 issue 的 log 确认 args 实际 shape，看是不是 schema mismatch；
- 如果是 schema mismatch → 在
callTool前加tool.inputSchema校验（治根）； - 如果还是要保留 sanitize 作兜底 → 用
unknown+JsonValue+ WeakSet + Date/Buffer/Error/toJSON显式分支，删掉外层try/catch。
如果你已经确认过根因是 Map/Set/BigInt（比如某个上游 transform 真的塞了这些进来），麻烦贴一下来源——我刚才扫了一下 callTool 的调用路径没看到这种情况，可能漏了。
|
Note This comment was translated by Claude. Looks like a server-side issue. Original Content看起来是服务端的问题 |
2 participants
What this PR does
Before this PR:
After this PR:
Fixes #14868
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to discussion: #14868
Breaking changes
None.
Special notes for your reviewer
Checklist
Release note