-
Notifications
You must be signed in to change notification settings - Fork 62
Tools can also return DocumentBlock, ImageBlock, VideoBlock #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
|---|---|---|
|
|
@@ -98,6 +98,28 @@ describe('tool', () => { | |
| const result = await myTool.invoke({ count: 3 }) | ||
| expect(result).toBe(3) | ||
| }) | ||
|
|
||
| it('handles DocumentBlock return', async () => { | ||
|
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. Can you add a similar test to the agent.test.ts file to add a tool which returns a document, and create a mock model to invoke that tool, then ensure the loop completes successfully? https://github.com/strands-agents/sdk-typescript/blob/main/src/agent/__tests__/agent.test.ts#L78
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. Test only verifies the structure but doesn't validate the actual bytes content was preserved. Consider adding assertion to decode and verify the bytes match the input: |
||
| const { DocumentBlock } = await import('../../types/media.js') | ||
|
|
||
| const docTool = tool({ | ||
| name: 'create_document', | ||
| description: 'Creates a document', | ||
| inputSchema: z.object({ content: z.string() }), | ||
| callback: (input) => { | ||
| return new DocumentBlock({ | ||
| name: 'RESULT', | ||
| format: 'md', | ||
| source: { bytes: new TextEncoder().encode(input.content) }, | ||
| }) | ||
| }, | ||
| }) | ||
|
|
||
| const result = await docTool.invoke({ content: 'Hello World!' }) | ||
| expect(result.type).toBe('documentBlock') | ||
| expect(result.name).toBe('RESULT') | ||
| expect(result.format).toBe('md') | ||
| }) | ||
|
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: Test coverage is incomplete for media block returns. Only Suggestion: Add similar tests for ImageBlock and VideoBlock: it('handles ImageBlock return', async () => {
const { ImageBlock } = await import('../../types/media.js')
const imgTool = tool({
name: 'create_image',
description: 'Creates an image',
inputSchema: z.object({ data: z.string() }),
callback: (input) => {
return new ImageBlock({
format: 'png',
source: { bytes: new TextEncoder().encode(input.data) },
})
},
})
const result = await imgTool.invoke({ data: 'test' })
expect(result.type).toBe('imageBlock')
expect(result.format).toBe('png')
}) |
||
| }) | ||
|
|
||
| describe('validation', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import type { ToolSpec } from './types.js' | |||||
| import type { JSONSchema, JSONValue } from '../types/json.js' | ||||||
| import { deepCopy } from '../types/json.js' | ||||||
| import { JsonBlock, TextBlock, ToolResultBlock } from '../types/messages.js' | ||||||
| import { DocumentBlock, ImageBlock, VideoBlock } from '../types/media.js' | ||||||
|
|
||||||
| /** | ||||||
| * Callback function for FunctionTool implementations. | ||||||
|
|
@@ -218,6 +219,15 @@ export class FunctionTool extends Tool { | |||||
| */ | ||||||
| private _wrapInToolResult(value: unknown, toolUseId: string): ToolResultBlock { | ||||||
| try { | ||||||
| // Handle DocumentBlock, ImageBlock, VideoBlock directly | ||||||
| if (value instanceof DocumentBlock || value instanceof ImageBlock || value instanceof VideoBlock) { | ||||||
|
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. Can we just do an instance check against ToolResultContent instead? That way
Suggested change
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. The instanceof check happens before the null check, but media blocks could theoretically be null in edge cases.
|
||||||
| return new ToolResultBlock({ | ||||||
| toolUseId, | ||||||
| status: 'success', | ||||||
| content: [value], | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| // Handle null with special string representation as text content | ||||||
| if (value === null) { | ||||||
| return new ToolResultBlock({ | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,13 @@ import type { ToolSpec } from './types.js' | |||||
| import type { JSONSchema, JSONValue } from '../types/json.js' | ||||||
| import { FunctionTool } from './function-tool.js' | ||||||
| import { z, ZodVoid } from 'zod' | ||||||
| import { DocumentBlock, ImageBlock, VideoBlock } from '../types/media.js' | ||||||
|
|
||||||
| /** | ||||||
| * Valid return types for tool callbacks. | ||||||
| * Includes JSON-serializable values and media blocks. | ||||||
| */ | ||||||
| export type ToolReturnValue = JSONValue | DocumentBlock | ImageBlock | VideoBlock | ||||||
|
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.
Suggested change
|
||||||
|
|
||||||
| /** | ||||||
| * Helper type to infer input type from Zod schema or default to never. | ||||||
|
|
@@ -16,7 +23,7 @@ type ZodInferred<TInput> = TInput extends z.ZodType ? z.infer<TInput> : never | |||||
| * @typeParam TInput - Zod schema type for input validation | ||||||
| * @typeParam TReturn - Return type of the callback function | ||||||
| */ | ||||||
| export interface ToolConfig<TInput extends z.ZodType | undefined, TReturn extends JSONValue = JSONValue> { | ||||||
| export interface ToolConfig<TInput extends z.ZodType | undefined, TReturn extends ToolReturnValue = JSONValue> { | ||||||
| /** The name of the tool */ | ||||||
| name: string | ||||||
|
|
||||||
|
|
@@ -46,7 +53,7 @@ export interface ToolConfig<TInput extends z.ZodType | undefined, TReturn extend | |||||
| * Internal implementation of a Zod-based tool. | ||||||
| * Extends Tool abstract class and implements InvokableTool interface. | ||||||
| */ | ||||||
| class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONValue = JSONValue> | ||||||
| class ZodTool<TInput extends z.ZodType | undefined, TReturn extends ToolReturnValue = JSONValue> | ||||||
| extends Tool | ||||||
| implements InvokableTool<ZodInferred<TInput>, TReturn> | ||||||
| { | ||||||
|
|
@@ -231,7 +238,7 @@ class ZodTool<TInput extends z.ZodType | undefined, TReturn extends JSONValue = | |||||
| * @param config - Tool configuration | ||||||
| * @returns An InvokableTool that implements the Tool interface with invoke() method | ||||||
| */ | ||||||
| export function tool<TInput extends z.ZodType | undefined, TReturn extends JSONValue = JSONValue>( | ||||||
| export function tool<TInput extends z.ZodType | undefined, TReturn extends ToolReturnValue = JSONValue>( | ||||||
| config: ToolConfig<TInput, TReturn> | ||||||
| ): InvokableTool<ZodInferred<TInput>, TReturn> { | ||||||
| return new ZodTool(config) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,9 +192,14 @@ export class ToolUseBlock implements ToolUseBlockData { | |
| * | ||
| * This is a discriminated union where the object key determines the content format. | ||
| */ | ||
| export type ToolResultContentData = TextBlockData | JsonBlockData | ||
| export type ToolResultContentData = | ||
| | TextBlockData | ||
| | JsonBlockData | ||
| | { document: DocumentBlockData } | ||
| | { image: ImageBlockData } | ||
| | { video: VideoBlockData } | ||
|
|
||
| export type ToolResultContent = TextBlock | JsonBlock | ||
| export type ToolResultContent = TextBlock | JsonBlock | DocumentBlock | ImageBlock | VideoBlock | ||
|
|
||
| /** | ||
| * Data for a tool result block. | ||
|
|
@@ -226,7 +231,7 @@ export interface ToolResultBlockData { | |
| /** | ||
| * Tool result content block. | ||
| */ | ||
| export class ToolResultBlock implements ToolResultBlockData { | ||
| export class ToolResultBlock { | ||
|
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. nit: Why no longer implement |
||
| /** | ||
| * Discriminator for tool result content. | ||
| */ | ||
|
|
@@ -596,6 +601,12 @@ export function contentBlockFromData(data: ContentBlockData): ContentBlock { | |
| return new TextBlock(contentItem.text) | ||
| } else if ('json' in contentItem) { | ||
| return new JsonBlock(contentItem) | ||
| } else if ('document' in contentItem) { | ||
| return new DocumentBlock(contentItem.document as DocumentBlockData) | ||
| } else if ('image' in contentItem) { | ||
| return new ImageBlock(contentItem.image as ImageBlockData) | ||
| } else if ('video' in contentItem) { | ||
| return new VideoBlock(contentItem.video as VideoBlockData) | ||
|
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. I think we can remove the type assertions and let TypeScript infer the types to ensure the data matches the expected shape |
||
| } else { | ||
| throw new Error('Unknown ToolResultContentData type') | ||
| } | ||
|
|
||
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.
nit: Lets just remove the default case so we implicitly skip if an unsupported content type is provided. Or we can warn and skip to be explicit.