mirror of
https://github.com/claude-code-best/claude-code.git
synced 2026-06-18 06:15:51 +00:00
fix: ExecuteExtraTool 加 schema 预校验防止 deferred 工具崩溃
模型通过 ExecuteExtraTool 调 CronCreate 时把字段名拼成 schedule(而非 cron),raw params 透传到 validateInput,input.cron 为 undefined,触 发 parseCronExpression 的 expr.trim() 抛 TypeError。所有参数组合同 样崩溃,与具体参数无关。 三层修复: - ExecuteTool.call 在调 validateInput 前先跑 targetTool.inputSchema. safeParse(鸭子类型跳过 MCP),失败时返回 formatZodValidationError 友好消息,成功时透传 parsed data 让 .default() 生效、strictObject 拦截多余字段。这是架构根治,覆盖所有 deferred 工具。 - CronCreateTool.validateInput 顶部加 typeof string 守卫,给模型精确 字段错误消息。 - parseCronExpression 顶部加 typeof string 守卫,覆盖 cronToHuman 等 所有调用者。 新增 4 个测试覆盖:cron undefined 输入返回 null、ExecuteTool schema 验证拒绝错字段名且 validateInput 不被触达、.default() 透传、MCP-like 工具跳过 schema 校验。 Co-Authored-By: glm-5.2 <zai-org@claude-code-best.win>
This commit is contained in:
@@ -10,6 +10,7 @@ import {
|
||||
} from 'src/Tool.js'
|
||||
import { lazySchema } from 'src/utils/lazySchema.js'
|
||||
import { createUserMessage } from 'src/utils/messages.js'
|
||||
import { formatZodValidationError } from 'src/utils/toolErrors.js'
|
||||
import {
|
||||
extractDiscoveredToolNames,
|
||||
isSearchExtraToolsEnabledOptimistic,
|
||||
@@ -121,6 +122,42 @@ export const ExecuteTool = buildTool({
|
||||
}
|
||||
}
|
||||
|
||||
// Schema-validate params against the target tool BEFORE delegating.
|
||||
// ExecuteExtraTool passes raw params straight from the model to
|
||||
// validateInput/call without re-running the target's zod schema, so a
|
||||
// wrong field name (e.g. 'schedule' instead of 'cron') or a missing
|
||||
// required field reaches the tool as undefined and the first
|
||||
// .trim()/.length/.split() crashes with "undefined is not an object".
|
||||
// CronCreateTool's .trim() crash was the reported symptom; centralizing
|
||||
// the check here covers every deferred tool without relying on each one
|
||||
// to defensively guard its own validateInput. Duck-typed so MCP tools
|
||||
// (whose schema is inputJSONSchema, not zod) skip this branch.
|
||||
const targetSchema = targetTool.inputSchema as
|
||||
| { safeParse?: (data: unknown) => unknown }
|
||||
| undefined
|
||||
if (targetSchema?.safeParse) {
|
||||
const parsed = targetSchema.safeParse(input.params) as
|
||||
| { success: true; data: Record<string, unknown> }
|
||||
| { success: false; error: z.ZodError }
|
||||
if (!parsed.success) {
|
||||
return {
|
||||
data: {
|
||||
result: null,
|
||||
tool_name: input.tool_name,
|
||||
},
|
||||
newMessages: [
|
||||
createUserMessage({
|
||||
content: formatZodValidationError(input.tool_name, parsed.error),
|
||||
}),
|
||||
],
|
||||
}
|
||||
}
|
||||
// Use parsed params going forward — picks up .default() values and
|
||||
// strips unknown keys for strictObject schemas so validateInput/call
|
||||
// never see fields they don't expect.
|
||||
input.params = parsed.data
|
||||
}
|
||||
|
||||
// Validate input before delegating — prevents crashes when the model
|
||||
// omits required params (e.g. TeamCreate without team_name →
|
||||
// sanitizeName(undefined).replace() TypeError).
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, test, expect } from 'bun:test'
|
||||
import { mock } from 'bun:test'
|
||||
import { z } from 'zod/v4'
|
||||
import { logMock } from '../../../../../../tests/mocks/log'
|
||||
import { debugMock } from '../../../../../../tests/mocks/debug'
|
||||
|
||||
@@ -36,7 +37,16 @@ mock.module('src/utils/searchExtraTools.js', () => ({
|
||||
isSearchExtraToolsToolAvailable: () => true,
|
||||
isSearchExtraToolsEnabled: async () => true,
|
||||
isToolReferenceBlock: () => false,
|
||||
extractDiscoveredToolNames: () => new Set(['TestTool', 'SecretTool']),
|
||||
// Mark every name as discovered so tests can exercise tools other than
|
||||
// TestTool/SecretTool without being blocked by the discovery guard.
|
||||
extractDiscoveredToolNames: () =>
|
||||
new Set([
|
||||
'TestTool',
|
||||
'SecretTool',
|
||||
'CronCreate',
|
||||
'WithDefaults',
|
||||
'McpTool',
|
||||
]),
|
||||
isDeferredToolsDeltaEnabled: () => false,
|
||||
getDeferredToolsDelta: () => null,
|
||||
}))
|
||||
@@ -52,6 +62,7 @@ mock.module('src/utils/messages.js', () => ({
|
||||
content,
|
||||
uuid: 'test-uuid',
|
||||
}),
|
||||
INTERRUPT_MESSAGE_FOR_TOOL_USE: '[Request interrupted]',
|
||||
}))
|
||||
|
||||
const { ExecuteTool } = await import('../ExecuteTool.js')
|
||||
@@ -92,6 +103,48 @@ function makeMockTool(name: string, callResult: unknown = 'ok') {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds a mock tool with a real zod inputSchema, mirroring how actual
|
||||
* deferred tools (e.g. CronCreateTool) expose their schema. Records the
|
||||
* params that reach call() so tests can assert what was delegated.
|
||||
*/
|
||||
function makeMockToolWithSchema(
|
||||
name: string,
|
||||
schema: z.ZodType,
|
||||
opts: {
|
||||
validateInput?: (input: Record<string, unknown>) => {
|
||||
result: boolean
|
||||
message?: string
|
||||
}
|
||||
} = {},
|
||||
) {
|
||||
const calls: Record<string, unknown>[] = []
|
||||
return {
|
||||
tool: {
|
||||
name,
|
||||
inputSchema: schema,
|
||||
call: async (input: Record<string, unknown>) => {
|
||||
calls.push(input)
|
||||
return { data: { ok: true, received: input } }
|
||||
},
|
||||
validateInput: opts.validateInput,
|
||||
checkPermissions: async () => ({ behavior: 'allow' as const }),
|
||||
isEnabled: () => true,
|
||||
isConcurrencySafe: () => true,
|
||||
isReadOnly: () => false,
|
||||
isMcp: false,
|
||||
userFacingName: () => name,
|
||||
renderToolUseMessage: () => `Running ${name}`,
|
||||
mapToolResultToToolResultBlockParam: (content: unknown, id: string) => ({
|
||||
tool_use_id: id,
|
||||
type: 'tool_result',
|
||||
content,
|
||||
}),
|
||||
},
|
||||
calls,
|
||||
}
|
||||
}
|
||||
|
||||
describe('ExecuteTool', () => {
|
||||
test('executes a target tool by name', async () => {
|
||||
const mockTarget = makeMockTool('TestTool', { result: 'success' })
|
||||
@@ -182,4 +235,117 @@ describe('ExecuteTool', () => {
|
||||
expect(ExecuteTool.searchHint).toContain('execute')
|
||||
expect(ExecuteTool.searchHint).toContain('tool')
|
||||
})
|
||||
|
||||
test('schema-validates params against target tool before delegating', async () => {
|
||||
// Reproduces the CronCreate bug class: model passes 'schedule' but the
|
||||
// schema requires 'cron'. Without the pre-validation, params reach
|
||||
// validateInput with cron=undefined and crash on .trim().
|
||||
const { tool, calls } = makeMockToolWithSchema(
|
||||
'CronCreate',
|
||||
z.strictObject({
|
||||
cron: z.string(),
|
||||
prompt: z.string(),
|
||||
}),
|
||||
{
|
||||
validateInput: input => {
|
||||
// Mirrors CronCreateTool.validateInput pre-fix behavior — would
|
||||
// crash on undefined.trim() if schema pre-validation lets bad
|
||||
// params through. The guard in ExecuteTool must prevent this.
|
||||
const cron = input.cron as string | undefined
|
||||
if (typeof cron !== 'string') {
|
||||
throw new TypeError(
|
||||
"undefined is not an object (evaluating 'cron.trim')",
|
||||
)
|
||||
}
|
||||
return { result: true }
|
||||
},
|
||||
},
|
||||
)
|
||||
const ctx = makeContext([tool])
|
||||
|
||||
const result = await ExecuteTool.call(
|
||||
{
|
||||
tool_name: 'CronCreate',
|
||||
params: { schedule: '*/5 * * * *', prompt: 'hi' },
|
||||
},
|
||||
ctx,
|
||||
async () => ({ behavior: 'allow' }),
|
||||
{ type: 'assistant', content: [], uuid: 'msg1' } as never,
|
||||
undefined,
|
||||
)
|
||||
|
||||
// Schema validation rejects the wrong field name and returns a model-
|
||||
// friendly error instead of crashing.
|
||||
expect(result.data).toEqual({
|
||||
result: null,
|
||||
tool_name: 'CronCreate',
|
||||
})
|
||||
expect(result.newMessages).toBeDefined()
|
||||
const message = result.newMessages![0].content as string
|
||||
// Model gets told both what was missing and what was unexpected.
|
||||
expect(message).toMatch(/cron/i)
|
||||
expect(message).toMatch(/schedule/i)
|
||||
// validateInput was never called, so no crash reached it.
|
||||
expect(calls.length).toBe(0)
|
||||
})
|
||||
|
||||
test('passes through parsed params to target tool, applying schema defaults', async () => {
|
||||
const { tool, calls } = makeMockToolWithSchema(
|
||||
'WithDefaults',
|
||||
z.strictObject({
|
||||
cron: z.string(),
|
||||
prompt: z.string(),
|
||||
recurring: z.boolean().default(true),
|
||||
}),
|
||||
)
|
||||
const ctx = makeContext([tool])
|
||||
|
||||
const result = await ExecuteTool.call(
|
||||
{
|
||||
// recurring intentionally omitted — schema default must fill it in.
|
||||
tool_name: 'WithDefaults',
|
||||
params: { cron: '*/5 * * * *', prompt: 'hi' },
|
||||
},
|
||||
ctx,
|
||||
async () => ({ behavior: 'allow' }),
|
||||
{ type: 'assistant', content: [], uuid: 'msg1' } as never,
|
||||
undefined,
|
||||
)
|
||||
|
||||
expect(result.data).toEqual({
|
||||
result: {
|
||||
ok: true,
|
||||
received: { cron: '*/5 * * * *', prompt: 'hi', recurring: true },
|
||||
},
|
||||
tool_name: 'WithDefaults',
|
||||
})
|
||||
expect(calls.length).toBe(1)
|
||||
// .default() applied — target tool sees recurring: true without
|
||||
// needing to defend against undefined itself.
|
||||
expect(calls[0]).toEqual({
|
||||
cron: '*/5 * * * *',
|
||||
prompt: 'hi',
|
||||
recurring: true,
|
||||
})
|
||||
})
|
||||
|
||||
test('skips schema validation for tools without safeParse (e.g. MCP)', async () => {
|
||||
// MCP tools expose inputJSONSchema, not zod — must not crash on
|
||||
// duck-typed schema check.
|
||||
const mockTarget = makeMockTool('McpTool', { result: 'ok' })
|
||||
const ctx = makeContext([mockTarget])
|
||||
|
||||
const result = await ExecuteTool.call(
|
||||
{ tool_name: 'McpTool', params: { anything: true } },
|
||||
ctx,
|
||||
async () => ({ behavior: 'allow' }),
|
||||
{ type: 'assistant', content: [], uuid: 'msg1' } as never,
|
||||
undefined,
|
||||
)
|
||||
|
||||
expect(result.data).toEqual({
|
||||
result: { result: 'ok' },
|
||||
tool_name: 'McpTool',
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -80,6 +80,19 @@ export const CronCreateTool = buildTool({
|
||||
return getCronFilePath()
|
||||
},
|
||||
async validateInput(input): Promise<ValidationResult> {
|
||||
// ExecuteExtraTool passes raw params through without re-running this
|
||||
// tool's inputSchema, so when the model uses a wrong field name (e.g.
|
||||
// 'schedule' instead of 'cron'), input.cron is undefined. parseCronExpression
|
||||
// would throw on .trim(undefined); catch here with a message that tells
|
||||
// the model which field is actually required.
|
||||
if (typeof input.cron !== 'string' || input.cron.length === 0) {
|
||||
return {
|
||||
result: false,
|
||||
message:
|
||||
"Missing required parameter 'cron' (5-field cron expression, e.g. '*/5 * * * *'). Check parameter names against the schema.",
|
||||
errorCode: 1,
|
||||
}
|
||||
}
|
||||
if (!parseCronExpression(input.cron)) {
|
||||
return {
|
||||
result: false,
|
||||
|
||||
@@ -94,6 +94,16 @@ describe('parseCronExpression', () => {
|
||||
test('returns null for non-numeric tokens', () => {
|
||||
expect(parseCronExpression('abc * * * *')).toBeNull()
|
||||
})
|
||||
|
||||
test('returns null for undefined input without throwing', () => {
|
||||
// CronCreateTool.validateInput receives raw params from ExecuteExtraTool;
|
||||
// when the model passes a wrong field name (e.g. 'schedule' instead of
|
||||
// 'cron'), input.cron is undefined. Calling .trim() on undefined crashes
|
||||
// with "undefined is not an object" — parseCronExpression must fail
|
||||
// gracefully so the tool layer can return a clear validation error.
|
||||
expect(parseCronExpression(undefined as unknown as string)).toBeNull()
|
||||
expect(parseCronExpression(null as unknown as string)).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
describe('field range validation', () => {
|
||||
|
||||
@@ -81,6 +81,12 @@ function expandField(field: string, range: FieldRange): number[] | null {
|
||||
* Returns null if invalid or unsupported syntax.
|
||||
*/
|
||||
export function parseCronExpression(expr: string): CronFields | null {
|
||||
// Defensive against non-string input: ExecuteExtraTool passes raw params
|
||||
// through to validateInput without re-running the target tool's schema, so
|
||||
// a wrong field name (e.g. 'schedule' instead of 'cron') surfaces here as
|
||||
// undefined. Without this guard, .trim() below throws "undefined is not an
|
||||
// object" — every CronCreate call from ExecuteExtraTool fails identically.
|
||||
if (typeof expr !== 'string') return null
|
||||
const parts = expr.trim().split(/\s+/)
|
||||
if (parts.length !== 5) return null
|
||||
|
||||
|
||||
Reference in New Issue
Block a user