From 99b9c6a4002d7fed415f9ce9e74174bb5a962889 Mon Sep 17 00:00:00 2001 From: claude-code-best Date: Thu, 18 Jun 2026 13:59:27 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20ExecuteExtraTool=20=E5=8A=A0=20schema=20?= =?UTF-8?q?=E9=A2=84=E6=A0=A1=E9=AA=8C=E9=98=B2=E6=AD=A2=20deferred=20?= =?UTF-8?q?=E5=B7=A5=E5=85=B7=E5=B4=A9=E6=BA=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 模型通过 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 --- .../src/tools/ExecuteTool/ExecuteTool.ts | 37 ++++ .../__tests__/ExecuteTool.runner.ts | 168 +++++++++++++++++- .../tools/ScheduleCronTool/CronCreateTool.ts | 13 ++ src/utils/__tests__/cron.test.ts | 10 ++ src/utils/cron.ts | 6 + 5 files changed, 233 insertions(+), 1 deletion(-) diff --git a/packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts b/packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts index d731713a6..21d8f3c27 100644 --- a/packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts +++ b/packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts @@ -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 } + | { 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). diff --git a/packages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.runner.ts b/packages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.runner.ts index c8ab3c8f8..2846049ff 100644 --- a/packages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.runner.ts +++ b/packages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.runner.ts @@ -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) => { + result: boolean + message?: string + } + } = {}, +) { + const calls: Record[] = [] + return { + tool: { + name, + inputSchema: schema, + call: async (input: Record) => { + 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', + }) + }) }) diff --git a/packages/builtin-tools/src/tools/ScheduleCronTool/CronCreateTool.ts b/packages/builtin-tools/src/tools/ScheduleCronTool/CronCreateTool.ts index 57f384e96..63feabf9e 100644 --- a/packages/builtin-tools/src/tools/ScheduleCronTool/CronCreateTool.ts +++ b/packages/builtin-tools/src/tools/ScheduleCronTool/CronCreateTool.ts @@ -80,6 +80,19 @@ export const CronCreateTool = buildTool({ return getCronFilePath() }, async validateInput(input): Promise { + // 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, diff --git a/src/utils/__tests__/cron.test.ts b/src/utils/__tests__/cron.test.ts index 5856b4b3d..6ed4ef947 100644 --- a/src/utils/__tests__/cron.test.ts +++ b/src/utils/__tests__/cron.test.ts @@ -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', () => { diff --git a/src/utils/cron.ts b/src/utils/cron.ts index bf71fbcad..31a15f618 100644 --- a/src/utils/cron.ts +++ b/src/utils/cron.ts @@ -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