fix: bugs (#885)
* fix: error output truncation (10KB→40KB) and MCP tool bugs - toolErrors.ts: increase error truncation limit from 10KB to 40KB Shell output can be up to 30KB, so 10KB was silently cutting off error logs from systemctl, apt, python, etc. - MCPTool: cache compiled AJV validators (was recompiling every call) - MCPTool: fix validateInput error message showing [object Object] - MCPTool: null-guard mapToolResultToToolResultBlockParam - MCPTool: explicit null check in isResultTruncated - ReadMcpResourceTool: null-guard mapToolResultToToolResultBlockParam Tests (84 passing): - src/utils/toolErrors.test.ts (13 tests) - src/tools/BashTool/commandSemantics.test.ts (24 tests) - src/tools/BashTool/utils.test.ts (32 tests) - src/tools/MCPTool/MCPTool.test.ts (15 tests) * fix: address review blockers from PR #885 Blocker 1: Fix abort path in callMCPTool - Previously returned { content: undefined } on AbortError, which masked the cancellation and caused mapToolResultToToolResultBlockParam to send empty content to the API as if it were a successful result. - Now converts abort errors to our AbortError class and re-throws, so the tool execution framework handles it properly (skips logging, creates is_error: true result with [Request interrupted by user for tool use]). Blocker 2: Fix memory leak in AJV validator cache - Changed compiledValidatorCache from Map to WeakMap so schemas from disconnected/refreshed MCP tools can be garbage collected instead of accumulating strong references indefinitely. Also: null guards now return descriptive indicators instead of empty strings, making it clear when content is unexpectedly missing. --------- Co-authored-by: FluxLuFFy <FluxLuFFy@users.noreply.github.com> Co-authored-by: Fix Bot <fix@openclaw.ai>
This commit is contained in:
@@ -55,6 +55,7 @@ import { type MCPProgress, MCPTool } from '../../tools/MCPTool/MCPTool.js'
|
|||||||
import { createMcpAuthTool } from '../../tools/McpAuthTool/McpAuthTool.js'
|
import { createMcpAuthTool } from '../../tools/McpAuthTool/McpAuthTool.js'
|
||||||
import { ReadMcpResourceTool } from '../../tools/ReadMcpResourceTool/ReadMcpResourceTool.js'
|
import { ReadMcpResourceTool } from '../../tools/ReadMcpResourceTool/ReadMcpResourceTool.js'
|
||||||
import { createAbortController } from '../../utils/abortController.js'
|
import { createAbortController } from '../../utils/abortController.js'
|
||||||
|
import { AbortError, isAbortError } from '../../utils/errors.js'
|
||||||
import { count } from '../../utils/array.js'
|
import { count } from '../../utils/array.js'
|
||||||
import {
|
import {
|
||||||
checkAndRefreshOAuthTokenIfNeeded,
|
checkAndRefreshOAuthTokenIfNeeded,
|
||||||
@@ -3283,11 +3284,18 @@ async function callMCPTool({
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// When the users hits esc, avoid logspew
|
// When the user hits esc, convert to our AbortError class so the tool
|
||||||
if (!(e instanceof Error) || e.name !== 'AbortError') {
|
// execution framework handles it properly (skips logging, creates
|
||||||
throw e
|
// is_error: true result with [Request interrupted by user for tool use]).
|
||||||
|
// Previously this returned { content: undefined }, which masked the
|
||||||
|
// cancellation and caused mapToolResultToToolResultBlockParam to send
|
||||||
|
// empty/undefined content to the API as if it were a successful result.
|
||||||
|
if (isAbortError(e)) {
|
||||||
|
throw new AbortError(
|
||||||
|
e instanceof Error ? e.message : 'Tool execution cancelled',
|
||||||
|
)
|
||||||
}
|
}
|
||||||
return { content: undefined }
|
throw e
|
||||||
} finally {
|
} finally {
|
||||||
// Always clear intervals
|
// Always clear intervals
|
||||||
if (progressInterval !== undefined) {
|
if (progressInterval !== undefined) {
|
||||||
|
|||||||
163
src/tools/BashTool/commandSemantics.test.ts
Normal file
163
src/tools/BashTool/commandSemantics.test.ts
Normal file
@@ -0,0 +1,163 @@
|
|||||||
|
import { describe, expect, test } from 'bun:test'
|
||||||
|
import { interpretCommandResult } from './commandSemantics.js'
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// interpretCommandResult — exit code semantics per command
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('interpretCommandResult', () => {
|
||||||
|
// --- Default semantics (most commands) ---
|
||||||
|
describe('default semantics', () => {
|
||||||
|
test('exit code 0 = success, no error', () => {
|
||||||
|
const result = interpretCommandResult('python script.py', 0, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
expect(result.message).toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 1 = error', () => {
|
||||||
|
const result = interpretCommandResult('python script.py', 1, '', '')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
expect(result.message).toContain('exit code 1')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 127 = command not found', () => {
|
||||||
|
const result = interpretCommandResult('foobar', 127, '', '')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
expect(result.message).toContain('127')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 126 = permission denied', () => {
|
||||||
|
const result = interpretCommandResult('./script.sh', 126, '', '')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
expect(result.message).toContain('126')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 130 = SIGINT (but not treated as interrupted here)', () => {
|
||||||
|
const result = interpretCommandResult('long-command', 130, '', '')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- grep: 0=matches, 1=no matches, 2+=error ---
|
||||||
|
describe('grep', () => {
|
||||||
|
test('exit code 0 = matches found (not error)', () => {
|
||||||
|
const result = interpretCommandResult('grep foo file.txt', 0, 'foo\n', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 1 = no matches (not error)', () => {
|
||||||
|
const result = interpretCommandResult('grep foo file.txt', 1, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
expect(result.message).toContain('No matches found')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 2 = real error', () => {
|
||||||
|
const result = interpretCommandResult('grep foo file.txt', 2, '', 'No such file')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- ripgrep: same as grep ---
|
||||||
|
describe('rg', () => {
|
||||||
|
test('exit code 1 = no matches (not error)', () => {
|
||||||
|
const result = interpretCommandResult('rg pattern', 1, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 2 = error', () => {
|
||||||
|
const result = interpretCommandResult('rg pattern', 2, '', '')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- find: 0=success, 1=partial, 2+=error ---
|
||||||
|
describe('find', () => {
|
||||||
|
test('exit code 0 = success', () => {
|
||||||
|
const result = interpretCommandResult('find . -name "*.ts"', 0, 'file.ts\n', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 1 = partial success (not error)', () => {
|
||||||
|
const result = interpretCommandResult('find . -name "*.ts"', 1, 'file.ts\n', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
expect(result.message).toContain('inaccessible')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 2 = error', () => {
|
||||||
|
const result = interpretCommandResult('find . -name "*.ts"', 2, '', 'Permission denied')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- diff: 0=same, 1=different, 2+=error ---
|
||||||
|
describe('diff', () => {
|
||||||
|
test('exit code 0 = files identical', () => {
|
||||||
|
const result = interpretCommandResult('diff a.txt b.txt', 0, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 1 = files differ (not error)', () => {
|
||||||
|
const result = interpretCommandResult('diff a.txt b.txt', 1, '< line1\n> line2', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
expect(result.message).toContain('differ')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('exit code 2 = error', () => {
|
||||||
|
const result = interpretCommandResult('diff a.txt b.txt', 2, '', 'No such file')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- test/[: 0=true, 1=false, 2+=error ---
|
||||||
|
describe('test and [', () => {
|
||||||
|
test('test exit code 0 = condition true', () => {
|
||||||
|
const result = interpretCommandResult('test -f file.txt', 0, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('test exit code 1 = condition false (not error)', () => {
|
||||||
|
const result = interpretCommandResult('test -f file.txt', 1, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
expect(result.message).toContain('false')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('[ exit code 1 = condition false (not error)', () => {
|
||||||
|
const result = interpretCommandResult('[ -f file.txt ]', 1, '', '')
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- Compound commands ---
|
||||||
|
describe('compound commands', () => {
|
||||||
|
test('last command determines semantics: grep last', () => {
|
||||||
|
const result = interpretCommandResult('cd /tmp && grep foo file.txt', 1, '', '')
|
||||||
|
// grep exit code 1 = no matches, not error
|
||||||
|
expect(result.isError).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('last command determines semantics: python last', () => {
|
||||||
|
const result = interpretCommandResult('cd /tmp && python script.py', 1, '', '')
|
||||||
|
// python exit code 1 = error
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// --- systemctl, apt, docker (real-world commands) ---
|
||||||
|
describe('system/service commands', () => {
|
||||||
|
test('systemctl failure = error', () => {
|
||||||
|
const result = interpretCommandResult('systemctl start nginx', 1, '', 'Job for nginx.service failed')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
expect(result.message).toContain('exit code 1')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('apt failure = error', () => {
|
||||||
|
const result = interpretCommandResult('apt install foo', 100, '', 'Unable to locate package')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('docker failure = error', () => {
|
||||||
|
const result = interpretCommandResult('docker run ubuntu', 1, '', 'Unable to find image')
|
||||||
|
expect(result.isError).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
})
|
||||||
212
src/tools/BashTool/utils.test.ts
Normal file
212
src/tools/BashTool/utils.test.ts
Normal file
@@ -0,0 +1,212 @@
|
|||||||
|
import { describe, expect, test } from 'bun:test'
|
||||||
|
import {
|
||||||
|
stripEmptyLines,
|
||||||
|
isImageOutput,
|
||||||
|
parseDataUri,
|
||||||
|
formatOutput,
|
||||||
|
createContentSummary,
|
||||||
|
} from './utils.js'
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// stripEmptyLines — removes leading/trailing blank lines
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('stripEmptyLines', () => {
|
||||||
|
test('strips leading empty lines', () => {
|
||||||
|
expect(stripEmptyLines('\n\n\nhello')).toBe('hello')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('strips trailing empty lines', () => {
|
||||||
|
expect(stripEmptyLines('hello\n\n\n')).toBe('hello')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('strips both ends', () => {
|
||||||
|
expect(stripEmptyLines('\n\nhello\n\n')).toBe('hello')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('preserves internal empty lines', () => {
|
||||||
|
expect(stripEmptyLines('a\n\nb')).toBe('a\n\nb')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('all empty lines returns empty string', () => {
|
||||||
|
expect(stripEmptyLines('\n\n\n')).toBe('')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('empty string returns empty string', () => {
|
||||||
|
expect(stripEmptyLines('')).toBe('')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('preserves whitespace-only lines in the middle', () => {
|
||||||
|
expect(stripEmptyLines('a\n \nb')).toBe('a\n \nb')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('single line no change', () => {
|
||||||
|
expect(stripEmptyLines('hello')).toBe('hello')
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// isImageOutput — detects base64 data URIs
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('isImageOutput', () => {
|
||||||
|
test('detects PNG data URI', () => {
|
||||||
|
expect(isImageOutput('data:image/png;base64,iVBORw0KGgo=')).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('detects JPEG data URI', () => {
|
||||||
|
expect(isImageOutput('data:image/jpeg;base64,/9j/4AAQ')).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('detects GIF data URI', () => {
|
||||||
|
expect(isImageOutput('data:image/gif;base64,R0lGODlhAQABAIAAAP')).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('detects SVG data URI', () => {
|
||||||
|
expect(isImageOutput('data:image/svg+xml;base64,PHN2Zz4=')).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('rejects plain text', () => {
|
||||||
|
expect(isImageOutput('hello world')).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('rejects empty string', () => {
|
||||||
|
expect(isImageOutput('')).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('rejects non-image data URI', () => {
|
||||||
|
expect(isImageOutput('data:text/plain;base64,aGVsbG8=')).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('rejects partial data URI', () => {
|
||||||
|
expect(isImageOutput('data:image/png,')).toBe(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// parseDataUri — extracts media type and base64 payload
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('parseDataUri', () => {
|
||||||
|
test('parses valid PNG data URI', () => {
|
||||||
|
const result = parseDataUri('data:image/png;base64,iVBORw0KGgo=')
|
||||||
|
expect(result).toEqual({
|
||||||
|
mediaType: 'image/png',
|
||||||
|
data: 'iVBORw0KGgo=',
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test('parses valid JPEG data URI', () => {
|
||||||
|
const result = parseDataUri('data:image/jpeg;base64,/9j/4AAQ')
|
||||||
|
expect(result).toEqual({
|
||||||
|
mediaType: 'image/jpeg',
|
||||||
|
data: '/9j/4AAQ',
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles whitespace around URI', () => {
|
||||||
|
const result = parseDataUri(' data:image/png;base64,abc123 ')
|
||||||
|
expect(result).toEqual({
|
||||||
|
mediaType: 'image/png',
|
||||||
|
data: 'abc123',
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns null for non-data URI', () => {
|
||||||
|
expect(parseDataUri('https://example.com/image.png')).toBeNull()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns null for empty string', () => {
|
||||||
|
expect(parseDataUri('')).toBeNull()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns null for incomplete data URI', () => {
|
||||||
|
expect(parseDataUri('data:image/png')).toBeNull()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns null for non-base64 data URI', () => {
|
||||||
|
expect(parseDataUri('data:text/plain,hello')).toBeNull()
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// formatOutput — truncates long output with line count
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('formatOutput', () => {
|
||||||
|
test('short output passes through unchanged', () => {
|
||||||
|
const result = formatOutput('line1\nline2\nline3')
|
||||||
|
expect(result.truncatedContent).toBe('line1\nline2\nline3')
|
||||||
|
expect(result.totalLines).toBe(3)
|
||||||
|
expect(result.isImage).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('empty output', () => {
|
||||||
|
const result = formatOutput('')
|
||||||
|
expect(result.truncatedContent).toBe('')
|
||||||
|
expect(result.totalLines).toBe(1)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('image output is passed through', () => {
|
||||||
|
const img = 'data:image/png;base64,iVBORw0KGgo='
|
||||||
|
const result = formatOutput(img)
|
||||||
|
expect(result.truncatedContent).toBe(img)
|
||||||
|
expect(result.totalLines).toBe(1)
|
||||||
|
expect(result.isImage).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('single line no trailing newline', () => {
|
||||||
|
const result = formatOutput('hello')
|
||||||
|
expect(result.totalLines).toBe(1)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// createContentSummary — MCP content block summaries
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('createContentSummary', () => {
|
||||||
|
test('summarizes text blocks', () => {
|
||||||
|
const content = [
|
||||||
|
{ type: 'text' as const, text: 'Hello world' },
|
||||||
|
]
|
||||||
|
const result = createContentSummary(content)
|
||||||
|
expect(result).toContain('MCP Result')
|
||||||
|
expect(result).toContain('1 text block')
|
||||||
|
expect(result).toContain('Hello world')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('summarizes image blocks', () => {
|
||||||
|
const content = [
|
||||||
|
{ type: 'image' as const, data: 'base64data', mimeType: 'image/png' },
|
||||||
|
]
|
||||||
|
const result = createContentSummary(content)
|
||||||
|
expect(result).toContain('1 image')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('summarizes mixed content', () => {
|
||||||
|
const content = [
|
||||||
|
{ type: 'text' as const, text: 'Description' },
|
||||||
|
{ type: 'image' as const, data: 'base64data', mimeType: 'image/png' },
|
||||||
|
{ type: 'text' as const, text: 'More text' },
|
||||||
|
]
|
||||||
|
const result = createContentSummary(content)
|
||||||
|
expect(result).toContain('1 image')
|
||||||
|
expect(result).toContain('2 text blocks')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('truncates long text preview at 200 chars', () => {
|
||||||
|
const longText = 'x'.repeat(300)
|
||||||
|
const content = [
|
||||||
|
{ type: 'text' as const, text: longText },
|
||||||
|
]
|
||||||
|
const result = createContentSummary(content)
|
||||||
|
expect(result).toContain('...')
|
||||||
|
expect(result).toContain('x'.repeat(200))
|
||||||
|
})
|
||||||
|
|
||||||
|
test('empty content array', () => {
|
||||||
|
const result = createContentSummary([])
|
||||||
|
expect(result).toContain('MCP Result')
|
||||||
|
})
|
||||||
|
})
|
||||||
132
src/tools/MCPTool/MCPTool.test.ts
Normal file
132
src/tools/MCPTool/MCPTool.test.ts
Normal file
@@ -0,0 +1,132 @@
|
|||||||
|
import { describe, expect, test, beforeEach } from 'bun:test'
|
||||||
|
import { MCPTool } from './MCPTool.js'
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// MCPTool.validateInput — AJV schema validation
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('MCPTool.validateInput', () => {
|
||||||
|
test('passes when no inputJSONSchema is set', async () => {
|
||||||
|
const tool = { ...MCPTool, inputJSONSchema: undefined }
|
||||||
|
const result = await tool.validateInput({ anything: 'goes' }, {} as never)
|
||||||
|
expect(result.result).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('validates against inputJSONSchema when set', async () => {
|
||||||
|
const schema = {
|
||||||
|
type: 'object' as const,
|
||||||
|
properties: {
|
||||||
|
name: { type: 'string' },
|
||||||
|
},
|
||||||
|
required: ['name'],
|
||||||
|
additionalProperties: false,
|
||||||
|
}
|
||||||
|
const tool = { ...MCPTool, inputJSONSchema: schema }
|
||||||
|
|
||||||
|
// Valid input
|
||||||
|
const valid = await tool.validateInput({ name: 'test' }, {} as never)
|
||||||
|
expect(valid.result).toBe(true)
|
||||||
|
|
||||||
|
// Missing required field
|
||||||
|
const invalid = await tool.validateInput({}, {} as never)
|
||||||
|
expect(invalid.result).toBe(false)
|
||||||
|
expect(invalid.result === false && invalid.message).toContain('name')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('rejects extra properties when additionalProperties is false', async () => {
|
||||||
|
const schema = {
|
||||||
|
type: 'object' as const,
|
||||||
|
properties: {
|
||||||
|
x: { type: 'number' },
|
||||||
|
},
|
||||||
|
additionalProperties: false,
|
||||||
|
}
|
||||||
|
const tool = { ...MCPTool, inputJSONSchema: schema }
|
||||||
|
|
||||||
|
const result = await tool.validateInput({ x: 1, extra: 'bad' }, {} as never)
|
||||||
|
expect(result.result).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles invalid schema gracefully', async () => {
|
||||||
|
// Schema that will cause ajv.compile to throw
|
||||||
|
const schema = { type: 'invalid_type' } as any
|
||||||
|
const tool = { ...MCPTool, inputJSONSchema: schema }
|
||||||
|
|
||||||
|
const result = await tool.validateInput({}, {} as never)
|
||||||
|
expect(result.result).toBe(false)
|
||||||
|
expect(result.result === false && result.errorCode).toBe(500)
|
||||||
|
expect(result.result === false && result.message).toContain('Failed to compile')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('error message is readable (not [object Object])', async () => {
|
||||||
|
const schema = { type: 'invalid_type' } as any
|
||||||
|
const tool = { ...MCPTool, inputJSONSchema: schema }
|
||||||
|
|
||||||
|
const result = await tool.validateInput({}, {} as never)
|
||||||
|
expect(result.result).toBe(false)
|
||||||
|
// Should NOT contain [object Object]
|
||||||
|
expect(result.result === false && result.message).not.toContain('[object Object]')
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// MCPTool.mapToolResultToToolResultBlockParam — null safety
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('MCPTool.mapToolResultToToolResultBlockParam', () => {
|
||||||
|
test('handles string content', () => {
|
||||||
|
const result = MCPTool.mapToolResultToToolResultBlockParam('hello', 'tool-1')
|
||||||
|
expect(result.content).toBe('hello')
|
||||||
|
expect(result.tool_use_id).toBe('tool-1')
|
||||||
|
expect(result.type).toBe('tool_result')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles array content', () => {
|
||||||
|
const blocks = [{ type: 'text', text: 'hello' }]
|
||||||
|
const result = MCPTool.mapToolResultToToolResultBlockParam(blocks as any, 'tool-2')
|
||||||
|
expect(result.content).toEqual(blocks)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles undefined content gracefully', () => {
|
||||||
|
const result = MCPTool.mapToolResultToToolResultBlockParam(undefined as any, 'tool-3')
|
||||||
|
expect(result.content).toBe('[No content returned from MCP tool]')
|
||||||
|
expect(result.tool_use_id).toBe('tool-3')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles null content gracefully', () => {
|
||||||
|
const result = MCPTool.mapToolResultToToolResultBlockParam(null as any, 'tool-4')
|
||||||
|
expect(result.content).toBe('[No content returned from MCP tool]')
|
||||||
|
expect(result.tool_use_id).toBe('tool-4')
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// MCPTool.isResultTruncated
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('MCPTool.isResultTruncated', () => {
|
||||||
|
test('returns false for short string', () => {
|
||||||
|
expect(MCPTool.isResultTruncated('short')).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns false for empty array', () => {
|
||||||
|
expect(MCPTool.isResultTruncated([])).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns false for array with short text blocks', () => {
|
||||||
|
expect(MCPTool.isResultTruncated([{ type: 'text', text: 'short' }])).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles null blocks in array', () => {
|
||||||
|
expect(MCPTool.isResultTruncated([null as any, { type: 'text', text: 'ok' }])).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('handles undefined blocks in array', () => {
|
||||||
|
expect(MCPTool.isResultTruncated([undefined as any])).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('returns false for non-string non-array', () => {
|
||||||
|
expect(MCPTool.isResultTruncated(42 as any)).toBe(false)
|
||||||
|
expect(MCPTool.isResultTruncated({} as any)).toBe(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -40,6 +40,21 @@ export type { MCPProgress } from '../../types/tools.js'
|
|||||||
|
|
||||||
const ajv = new Ajv({ strict: false })
|
const ajv = new Ajv({ strict: false })
|
||||||
|
|
||||||
|
// Cache compiled validators to avoid recompiling on every validateInput call.
|
||||||
|
// AJV compilation is expensive — schemas don't change between calls.
|
||||||
|
// Uses WeakMap to allow garbage collection of schemas from disconnected/refreshed
|
||||||
|
// MCP tools, preventing memory leaks from accumulating strong references.
|
||||||
|
const compiledValidatorCache = new WeakMap<object, ReturnType<typeof ajv.compile>>()
|
||||||
|
|
||||||
|
function getCompiledValidator(schema: object) {
|
||||||
|
let validator = compiledValidatorCache.get(schema)
|
||||||
|
if (!validator) {
|
||||||
|
validator = ajv.compile(schema)
|
||||||
|
compiledValidatorCache.set(schema, validator)
|
||||||
|
}
|
||||||
|
return validator
|
||||||
|
}
|
||||||
|
|
||||||
export const MCPTool = buildTool({
|
export const MCPTool = buildTool({
|
||||||
isMcp: true,
|
isMcp: true,
|
||||||
// Overridden in mcpClient.ts with the real MCP tool name + args
|
// Overridden in mcpClient.ts with the real MCP tool name + args
|
||||||
@@ -78,7 +93,7 @@ export const MCPTool = buildTool({
|
|||||||
async validateInput(input, context): Promise<ValidationResult> {
|
async validateInput(input, context): Promise<ValidationResult> {
|
||||||
if (this.inputJSONSchema) {
|
if (this.inputJSONSchema) {
|
||||||
try {
|
try {
|
||||||
const validate = ajv.compile(this.inputJSONSchema)
|
const validate = getCompiledValidator(this.inputJSONSchema)
|
||||||
if (!validate(input)) {
|
if (!validate(input)) {
|
||||||
return {
|
return {
|
||||||
result: false,
|
result: false,
|
||||||
@@ -87,9 +102,10 @@ export const MCPTool = buildTool({
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
const errMsg = error instanceof Error ? error.message : String(error)
|
||||||
return {
|
return {
|
||||||
result: false,
|
result: false,
|
||||||
message: `Failed to compile JSON schema for validation: ${error}`,
|
message: `Failed to compile JSON schema for validation: ${errMsg}`,
|
||||||
errorCode: 500,
|
errorCode: 500,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -109,7 +125,8 @@ export const MCPTool = buildTool({
|
|||||||
if (Array.isArray(output)) {
|
if (Array.isArray(output)) {
|
||||||
return output.some(
|
return output.some(
|
||||||
block =>
|
block =>
|
||||||
block?.type === 'text' &&
|
block != null &&
|
||||||
|
block.type === 'text' &&
|
||||||
typeof block.text === 'string' &&
|
typeof block.text === 'string' &&
|
||||||
isOutputLineTruncated(block.text),
|
isOutputLineTruncated(block.text),
|
||||||
)
|
)
|
||||||
@@ -117,6 +134,16 @@ export const MCPTool = buildTool({
|
|||||||
return false
|
return false
|
||||||
},
|
},
|
||||||
mapToolResultToToolResultBlockParam(content, toolUseID) {
|
mapToolResultToToolResultBlockParam(content, toolUseID) {
|
||||||
|
// Defensive guard: if content is undefined/null (shouldn't happen after
|
||||||
|
// the abort path fix in client.ts), return a clear indicator rather than
|
||||||
|
// sending undefined to the API which would cause an error.
|
||||||
|
if (content === undefined || content === null) {
|
||||||
|
return {
|
||||||
|
tool_use_id: toolUseID,
|
||||||
|
type: 'tool_result',
|
||||||
|
content: '[No content returned from MCP tool]',
|
||||||
|
}
|
||||||
|
}
|
||||||
return {
|
return {
|
||||||
tool_use_id: toolUseID,
|
tool_use_id: toolUseID,
|
||||||
type: 'tool_result',
|
type: 'tool_result',
|
||||||
|
|||||||
@@ -149,6 +149,15 @@ export const ReadMcpResourceTool = buildTool({
|
|||||||
return isOutputLineTruncated(jsonStringify(output))
|
return isOutputLineTruncated(jsonStringify(output))
|
||||||
},
|
},
|
||||||
mapToolResultToToolResultBlockParam(content, toolUseID) {
|
mapToolResultToToolResultBlockParam(content, toolUseID) {
|
||||||
|
// Defensive guard: if content is undefined/null, return a clear indicator
|
||||||
|
// rather than sending undefined to jsonStringify which would cause an error.
|
||||||
|
if (content === undefined || content === null) {
|
||||||
|
return {
|
||||||
|
tool_use_id: toolUseID,
|
||||||
|
type: 'tool_result',
|
||||||
|
content: '[No content returned from MCP resource]',
|
||||||
|
}
|
||||||
|
}
|
||||||
return {
|
return {
|
||||||
tool_use_id: toolUseID,
|
tool_use_id: toolUseID,
|
||||||
type: 'tool_result',
|
type: 'tool_result',
|
||||||
|
|||||||
113
src/utils/toolErrors.test.ts
Normal file
113
src/utils/toolErrors.test.ts
Normal file
@@ -0,0 +1,113 @@
|
|||||||
|
import { describe, expect, test } from 'bun:test'
|
||||||
|
import { ShellError, AbortError } from './errors.js'
|
||||||
|
import { formatError, getErrorParts } from './toolErrors.js'
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// getErrorParts — what the model sees when a tool fails
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('getErrorParts', () => {
|
||||||
|
test('ShellError: exit code + stderr + stdout', () => {
|
||||||
|
const err = new ShellError('output here', 'error here', 1, false)
|
||||||
|
const parts = getErrorParts(err)
|
||||||
|
expect(parts).toEqual([
|
||||||
|
'Exit code 1',
|
||||||
|
'',
|
||||||
|
'error here',
|
||||||
|
'output here',
|
||||||
|
])
|
||||||
|
})
|
||||||
|
|
||||||
|
test('ShellError: interrupted flag adds interrupt message', () => {
|
||||||
|
const err = new ShellError('', 'partial output', 130, true)
|
||||||
|
const parts = getErrorParts(err)
|
||||||
|
expect(parts[0]).toBe('Exit code 130')
|
||||||
|
expect(parts[1]).toContain('interrupted')
|
||||||
|
expect(parts[2]).toBe('partial output')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('ShellError: empty stderr and stdout', () => {
|
||||||
|
const err = new ShellError('', '', 1, false)
|
||||||
|
const parts = getErrorParts(err)
|
||||||
|
expect(parts[0]).toBe('Exit code 1')
|
||||||
|
expect(parts[2]).toBe('')
|
||||||
|
expect(parts[3]).toBe('')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('non-ShellError: returns message + stderr + stdout if present', () => {
|
||||||
|
const err = new Error('something broke')
|
||||||
|
;(err as any).stderr = 'stderr data'
|
||||||
|
;(err as any).stdout = 'stdout data'
|
||||||
|
const parts = getErrorParts(err)
|
||||||
|
expect(parts[0]).toBe('something broke')
|
||||||
|
expect(parts[1]).toBe('stderr data')
|
||||||
|
expect(parts[2]).toBe('stdout data')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('non-ShellError: message only when no stderr/stdout', () => {
|
||||||
|
const err = new Error('just a message')
|
||||||
|
const parts = getErrorParts(err)
|
||||||
|
expect(parts).toEqual(['just a message'])
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
// =============================================================================
|
||||||
|
// formatError — final string sent to the model
|
||||||
|
// =============================================================================
|
||||||
|
|
||||||
|
describe('formatError', () => {
|
||||||
|
test('AbortError returns message or interrupt default', () => {
|
||||||
|
const err = new AbortError('user cancelled')
|
||||||
|
expect(formatError(err)).toBe('user cancelled')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('AbortError with empty message returns default interrupt', () => {
|
||||||
|
const err = new AbortError('')
|
||||||
|
const result = formatError(err)
|
||||||
|
expect(result.length).toBeGreaterThan(0)
|
||||||
|
})
|
||||||
|
|
||||||
|
test('non-Error value stringified', () => {
|
||||||
|
expect(formatError('raw string')).toBe('raw string')
|
||||||
|
expect(formatError(42)).toBe('42')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('ShellError: combines exit code + stderr + stdout', () => {
|
||||||
|
const err = new ShellError('stdout content', 'stderr content', 1, false)
|
||||||
|
const result = formatError(err)
|
||||||
|
expect(result).toContain('Exit code 1')
|
||||||
|
expect(result).toContain('stderr content')
|
||||||
|
expect(result).toContain('stdout content')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('ShellError: empty output falls back to default message', () => {
|
||||||
|
const err = new ShellError('', '', 1, false)
|
||||||
|
const result = formatError(err)
|
||||||
|
expect(result).toBe('Exit code 1')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('non-ShellError: message only', () => {
|
||||||
|
const err = new Error('something failed')
|
||||||
|
expect(formatError(err)).toBe('something failed')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('truncates at 40KB (not 10KB)', () => {
|
||||||
|
// 50KB of output — should be truncated at 40KB limit
|
||||||
|
const longOutput = 'x'.repeat(50_000)
|
||||||
|
const err = new ShellError('', longOutput, 1, false)
|
||||||
|
const result = formatError(err)
|
||||||
|
expect(result.length).toBeLessThan(50_000)
|
||||||
|
expect(result).toContain('truncated')
|
||||||
|
// Should keep first 20KB + last 20KB
|
||||||
|
expect(result).toContain('x'.repeat(100))
|
||||||
|
})
|
||||||
|
|
||||||
|
test('does NOT truncate under 40KB', () => {
|
||||||
|
// 30KB of output — should NOT be truncated
|
||||||
|
const output = 'y'.repeat(30_000)
|
||||||
|
const err = new ShellError('', output, 1, false)
|
||||||
|
const result = formatError(err)
|
||||||
|
expect(result).not.toContain('truncated')
|
||||||
|
expect(result).toContain(output)
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -12,13 +12,15 @@ export function formatError(error: unknown): string {
|
|||||||
const parts = getErrorParts(error)
|
const parts = getErrorParts(error)
|
||||||
const fullMessage =
|
const fullMessage =
|
||||||
parts.filter(Boolean).join('\n').trim() || 'Command failed with no output'
|
parts.filter(Boolean).join('\n').trim() || 'Command failed with no output'
|
||||||
if (fullMessage.length <= 10000) {
|
// 40KB limit — enough for most command error logs (systemctl, apt, python, etc.)
|
||||||
|
const maxErrorLength = 40000
|
||||||
|
if (fullMessage.length <= maxErrorLength) {
|
||||||
return fullMessage
|
return fullMessage
|
||||||
}
|
}
|
||||||
const halfLength = 5000
|
const halfLength = Math.floor(maxErrorLength / 2)
|
||||||
const start = fullMessage.slice(0, halfLength)
|
const start = fullMessage.slice(0, halfLength)
|
||||||
const end = fullMessage.slice(-halfLength)
|
const end = fullMessage.slice(-halfLength)
|
||||||
return `${start}\n\n... [${fullMessage.length - 10000} characters truncated] ...\n\n${end}`
|
return `${start}\n\n... [${fullMessage.length - maxErrorLength} characters truncated] ...\n\n${end}`
|
||||||
}
|
}
|
||||||
|
|
||||||
export function getErrorParts(error: Error): string[] {
|
export function getErrorParts(error: Error): string[] {
|
||||||
|
|||||||
Reference in New Issue
Block a user