-
Notifications
You must be signed in to change notification settings - Fork 102
feat: support deriving tools from existing tools via inputSchema #1086
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { describe, it, expectTypeOf } from 'vitest' | ||
| import { z } from 'zod' | ||
| import { tool } from '../tool-factory.js' | ||
|
|
||
| describe('tool()', () => { | ||
| describe('derived tool', () => { | ||
| const baseTool = tool({ | ||
| name: 'base', | ||
| inputSchema: z.object({ url: z.string(), method: z.enum(['GET', 'POST']) }), | ||
| callback: (input) => ({ fetched: input.url }), | ||
| }) | ||
|
|
||
| it('infers input and return types from source tool', () => { | ||
| const derived = tool({ | ||
| name: 'derived', | ||
| inputSchema: baseTool, | ||
| callback: (input) => ({ combined: `${input.method} ${input.url}` }), | ||
| }) | ||
|
|
||
| expectTypeOf(derived.invoke).parameter(0).toEqualTypeOf<{ url: string; method: 'GET' | 'POST' }>() | ||
| expectTypeOf(derived.invoke).returns.resolves.toEqualTypeOf<{ combined: string }>() | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,4 +110,79 @@ describe('tool factory', () => { | |
| expect(myTool.description).toBe('') | ||
| }) | ||
| }) | ||
|
|
||
| describe('DerivedTool (inputSchema as existing tool)', () => { | ||
| const zodTool = tool({ | ||
| name: 'zod_tool', | ||
| description: 'A Zod schema tool', | ||
| inputSchema: z.object({ url: z.string().url(), method: z.enum(['GET', 'POST']) }), | ||
| callback: async (input) => ({ fetched: input.url, method: input.method }), | ||
| }) | ||
|
|
||
| const functionTool = tool({ | ||
| name: 'function_tool', | ||
| description: 'A JSON schema tool', | ||
| inputSchema: { type: 'object', properties: { x: { type: 'number' } } }, | ||
| callback: (input) => `got ${(input as { x: number }).x}`, | ||
| }) | ||
|
|
||
| it('inherits input schema and defaults description from source tool', () => { | ||
| const derived = tool({ | ||
| name: 'derived_tool', | ||
| inputSchema: zodTool, | ||
| callback: async (input) => input.url, | ||
| }) | ||
|
|
||
| expect(derived).toBeInstanceOf(Tool) | ||
| expect(derived.name).toBe('derived_tool') | ||
| expect(derived.description).toBe('A Zod schema tool') | ||
| expect(derived.toolSpec.inputSchema).toStrictEqual(zodTool.toolSpec.inputSchema) | ||
| }) | ||
|
|
||
| it('overrides description when provided', () => { | ||
| const derived = tool({ | ||
| name: 'derived', | ||
| description: 'Custom description', | ||
| inputSchema: zodTool, | ||
| callback: async (input) => input.url, | ||
| }) | ||
|
|
||
| expect(derived.description).toBe('Custom description') | ||
| }) | ||
|
|
||
| it('delegates to source tool with typed input', async () => { | ||
| const derived = tool({ | ||
| name: 'derived', | ||
| inputSchema: zodTool, | ||
| callback: async (input, context) => { | ||
| const result = await zodTool.invoke(input, context) | ||
| return { ...result, wrapped: true } | ||
| }, | ||
| }) | ||
|
|
||
| const result = await derived.invoke({ url: 'https://example.com', method: 'POST' }) | ||
| expect(result).toStrictEqual({ fetched: 'https://example.com', method: 'POST', wrapped: true }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Missing test for Zod validation preservation in derived tools. The PR description states "When the source tool is a ZodTool, the derived tool preserves Zod runtime validation," but there's no test verifying that invalid input is rejected. This is the key behavioral claim of the ZodTool derivation path. Suggestion: Add a test that passes invalid input and asserts the Zod error: it('preserves Zod validation from source ZodTool', async () => {
const derived = tool({
name: 'derived',
inputSchema: zodTool,
callback: async (input) => input.url,
})
await expect(derived.invoke({ url: 'not-a-url', method: 'GET' })).rejects.toThrow()
}) |
||
| }) | ||
|
|
||
| it('inherits input schema from a FunctionTool source', async () => { | ||
| const derived = tool({ | ||
| name: 'derived_json', | ||
| inputSchema: functionTool, | ||
| callback: (input) => `wrapped: ${(input as { x: number }).x}`, | ||
| }) | ||
|
|
||
| expect(derived.toolSpec.inputSchema).toStrictEqual(functionTool.toolSpec.inputSchema) | ||
| expect(await derived.invoke({ x: 42 })).toBe('wrapped: 42') | ||
| }) | ||
|
|
||
| it('preserves Zod validation from source ZodTool', async () => { | ||
| const derived = tool({ | ||
| name: 'derived', | ||
| inputSchema: zodTool, | ||
| callback: async (input) => input.url, | ||
| }) | ||
|
|
||
| await expect(derived.invoke({ url: 'not-a-url', method: 'GET' })).rejects.toThrow() | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import type { InvokableTool } from './tool.js' | ||
| import type { InvokableTool, ToolContext } from './tool.js' | ||
| import { Tool } from './tool.js' | ||
| import { FunctionTool } from './function-tool.js' | ||
| import type { FunctionToolConfig } from './function-tool.js' | ||
| import type { JSONValue } from '../types/json.js' | ||
|
|
@@ -15,6 +16,29 @@ function isZodType(value: unknown): value is z.ZodType { | |
| return value instanceof z.ZodType | ||
| } | ||
|
|
||
| /** | ||
| * Configuration for creating a tool that reuses another tool's input schema. | ||
| * | ||
| * @typeParam TInput - Input type inferred from the source tool | ||
| * @typeParam TReturn - Return type of the callback function | ||
| */ | ||
| interface DerivedToolConfig<TInput, TReturn extends JSONValue> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The Suggestion: Export
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Suggestion: Export |
||
| /** The unique name of the tool */ | ||
| name: string | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to inherit
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered it but figured it would be good to force it at least for now since a derived tool is something different.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that it's agent-visible my gut says that most would want to keep the name. Like I don't expect both tools being passed to an agent being a common use-case. Not a blocker FWIW. |
||
|
|
||
| /** Human-readable description of the tool's purpose. Defaults to the source tool's description. */ | ||
| description?: string | ||
|
|
||
| /** An existing tool whose input schema and types will be reused */ | ||
| inputSchema: InvokableTool<TInput, unknown> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The The existing reviewers raised a similar point about whether a Suggestion: Consider whether a separate field name (e.g., |
||
|
|
||
| /** Function that implements the tool logic with typed input from the source tool */ | ||
| callback: ( | ||
| input: TInput, | ||
| context?: ToolContext | ||
| ) => AsyncGenerator<JSONValue, TReturn, never> | Promise<TReturn> | TReturn | ||
| } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each tool type has a config defined like this (example). |
||
|
|
||
| /** | ||
| * Creates an InvokableTool from a Zod schema and callback function. | ||
| * | ||
|
|
@@ -36,11 +60,26 @@ export function tool<TInput extends z.ZodType, TReturn extends JSONValue = JSONV | |
| export function tool(config: FunctionToolConfig): InvokableTool<unknown, JSONValue> | ||
|
|
||
| /** | ||
| * Creates an InvokableTool from either a Zod schema or JSON schema configuration. | ||
| * Creates an InvokableTool that reuses another tool's input schema. | ||
| * The callback receives the same typed input as the source tool. | ||
| * | ||
| * @typeParam TInput - Input type inferred from the source tool | ||
| * @typeParam TReturn - Return type of the callback function | ||
| * @param config - Tool configuration with a source tool as inputSchema | ||
| * @returns An InvokableTool with input typed from the source tool | ||
| */ | ||
| export function tool<TInput, TReturn extends JSONValue = JSONValue>( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we consider anything like a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I guess we can't have a helper method on tools because they're interfaces could we? otherwise this devx would be cool and fairly cool:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just didn't want to give the users a separate export. I thought it might be simpler to overload. But definitely open to suggestions here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of the options that fit our criteria, tool.wrap(httpTool, { ... }Might be the most clear - it guides you towards thinking of it as "inheriting" or "wrapping". But either way, I don't expect most folks to self-discover this, so I think it's going to be a docs thing; but
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The overload ordering places Consider placing the |
||
| config: DerivedToolConfig<TInput, TReturn> | ||
| ): InvokableTool<TInput, TReturn> | ||
|
|
||
| /** | ||
| * Creates an InvokableTool from a Zod schema, JSON schema, or existing tool. | ||
| * | ||
| * When a Zod schema is provided as `inputSchema`, input is validated at runtime and | ||
| * the callback receives typed input. When a JSON schema (or no schema) is provided, | ||
| * the callback receives `unknown` input with no runtime validation. | ||
| * the callback receives typed input. When an existing tool is provided as `inputSchema`, | ||
| * the callback receives input typed from that tool's generic parameters. When a JSON | ||
| * schema (or no schema) is provided, the callback receives `unknown` input with no | ||
| * runtime validation. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
|
|
@@ -66,14 +105,45 @@ export function tool(config: FunctionToolConfig): InvokableTool<unknown, JSONVal | |
| * }, | ||
| * callback: (input) => `Hello, ${(input as { name: string }).name}!`, | ||
| * }) | ||
| * | ||
| * // With existing tool (typed from source tool) | ||
| * const positiveCalculator = tool({ | ||
| * name: 'positive_calculator', | ||
| * description: 'Adds two positive numbers', | ||
| * inputSchema: calculator, | ||
| * callback: (input) => { | ||
| * if (input.a < 0 || input.b < 0) throw new Error('No negatives') | ||
| * return calculator.invoke(input) | ||
| * }, | ||
| * }) | ||
| * ``` | ||
| * | ||
| * @param config - Tool configuration | ||
| * @returns An InvokableTool that implements the Tool interface with invoke() method | ||
| */ | ||
| export function tool( | ||
| config: ZodToolConfig<z.ZodType | undefined, JSONValue> | FunctionToolConfig | ||
| config: ZodToolConfig<z.ZodType | undefined, JSONValue> | FunctionToolConfig | DerivedToolConfig<unknown, JSONValue> | ||
| ): InvokableTool<unknown, JSONValue> { | ||
| if (config.inputSchema && config.inputSchema instanceof Tool) { | ||
| const sourceTool = config.inputSchema | ||
|
|
||
| if (sourceTool instanceof ZodTool) { | ||
| return new ZodTool({ | ||
| name: config.name, | ||
| description: config.description ?? sourceTool.description, | ||
| inputSchema: sourceTool.inputSchema, | ||
| callback: config.callback, | ||
| } as ZodToolConfig<z.ZodType, JSONValue>) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The When deriving from a source tool, the Suggestion: Consider extracting the callback assignment explicitly so the type relationship is clear, or add a comment explaining why the cast is safe here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: The The Suggestion: Consider a thin wrapper function that explicitly adapts the callback signature instead of relying on const wrappedCallback: FunctionToolCallback = (input, toolContext) =>
(config.callback as DerivedToolConfig<unknown, JSONValue>['callback'])(input, toolContext)This makes the adaptation explicit rather than relying on structural compatibility hidden behind a cast. |
||
| } | ||
|
|
||
| return new FunctionTool({ | ||
| name: config.name, | ||
| description: config.description ?? sourceTool.description, | ||
| inputSchema: sourceTool.toolSpec.inputSchema, | ||
| callback: config.callback, | ||
| } as FunctionToolConfig) | ||
| } | ||
|
|
||
| if (config.inputSchema && isZodType(config.inputSchema)) { | ||
| return new ZodTool(config as ZodToolConfig<z.ZodType, JSONValue>) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,9 +58,8 @@ export class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONV | |
|
|
||
| /** | ||
| * Zod schema for input validation. | ||
| * Note: undefined is normalized to z.void() in constructor, so this is always defined. | ||
| */ | ||
| private readonly _inputSchema: z.ZodType | ||
| readonly inputSchema: TInput extends z.ZodType ? TInput : z.ZodVoid | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Exposing Previously Additionally, the property type ( Suggestion: If the only consumer of this property is the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users might want to access it for custom type inference, testing, or building their own composition utilities. Also, we already expose Zod through the public contract. This is the ZodTool after all.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Exposing Previously Additionally, the property type ( Suggestion: If the only consumer is the |
||
|
|
||
| /** | ||
| * User callback function. | ||
|
|
@@ -75,20 +74,20 @@ export class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONV | |
| const { name, description = '', inputSchema, callback } = config | ||
|
|
||
| // Normalize undefined to z.void() to simplify logic throughout | ||
| this._inputSchema = inputSchema ?? z.void() | ||
| this.inputSchema = (inputSchema ?? z.void()) as TInput extends z.ZodType ? TInput : z.ZodVoid | ||
| this._callback = callback | ||
|
|
||
| let generatedSchema: JSONSchema | ||
|
|
||
| // Handle z.void() - use default empty object schema | ||
| if (this._inputSchema instanceof ZodVoid) { | ||
| if (this.inputSchema instanceof ZodVoid) { | ||
| generatedSchema = { | ||
| type: 'object', | ||
| properties: {}, | ||
| additionalProperties: false, | ||
| } | ||
| } else { | ||
| generatedSchema = zodSchemaToJsonSchema(this._inputSchema) | ||
| generatedSchema = zodSchemaToJsonSchema(this.inputSchema) | ||
| } | ||
|
|
||
| // Create a FunctionTool with a validation wrapper | ||
|
|
@@ -101,7 +100,7 @@ export class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONV | |
| toolContext: ToolContext | ||
| ): AsyncGenerator<JSONValue, JSONValue, never> | Promise<JSONValue> | JSONValue => { | ||
| // Only validate if schema is not z.void() (after normalization, it's never undefined) | ||
| const validatedInput = this._inputSchema instanceof ZodVoid ? input : this._inputSchema.parse(input) | ||
| const validatedInput = this.inputSchema instanceof ZodVoid ? input : this.inputSchema.parse(input) | ||
| // Execute user callback with validated input | ||
| return callback(validatedInput as ZodInferred<TInput>, toolContext) as | ||
| | AsyncGenerator<JSONValue, JSONValue, never> | ||
|
|
@@ -157,7 +156,7 @@ export class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONV | |
| */ | ||
| async invoke(input: ZodInferred<TInput>, context?: ToolContext): Promise<TReturn> { | ||
| // Only validate if schema is not z.void() (after normalization, it's never undefined) | ||
| const validatedInput = this._inputSchema instanceof ZodVoid ? input : this._inputSchema.parse(input) | ||
| const validatedInput = this.inputSchema instanceof ZodVoid ? input : this.inputSchema.parse(input) | ||
|
|
||
| // Execute callback with validated input | ||
| const result = this._callback(validatedInput as ZodInferred<TInput>, context) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Missing test for Zod validation preservation in derived tools.
The PR description states "When the source tool is a ZodTool, the derived tool preserves Zod runtime validation," but there's no test that passes invalid input to a derived ZodTool and asserts that it throws a Zod validation error. This is the key differentiator from the FunctionTool path and should be explicitly tested.
Suggestion: Add a test like: