From 5fd1dbdc36d393eb15a15b51bb12c652f066ea65 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 18:44:29 -0700 Subject: [PATCH 1/2] [codex] Clarify string tool input errors (#599) --- .../__tests__/tool-validation-error.test.ts | 45 +++++++++++++++++-- .../agent-runtime/src/tools/tool-executor.ts | 6 +-- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts index ed5cfaa5a..520b4d087 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -239,8 +239,46 @@ describe('tool validation error handling', () => { expect('error' in result).toBe(true) if ('error' in result) { - expect(result.error).toContain('The JSON parser reported:') - expect(result.error).toContain('If the arguments are incomplete') + expect(result.error).toContain( + 'expected the tool arguments to be an object, but received a string', + ) + expect(result.error).toContain('Parsing as JSON failed:') + expect(result.error).toContain( + 'The arguments may be malformed or incomplete', + ) + } + }) + + it('should explain when parsed tool input remains a string', () => { + const input = JSON.stringify( + JSON.stringify( + JSON.stringify( + JSON.stringify({ + path: 'test.ts', + instructions: 'Writes a test file', + content: 'console.log("test")\n', + }), + ), + ), + ) + + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'write_file', + toolCallId: 'over-encoded-tool-call-id', + input, + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain( + 'expected the tool arguments to be an object, but received a string', + ) + expect(result.error).toContain( + 'Parsing succeeded, but the parsed value was still a string', + ) + expect(result.error).not.toContain('malformed or incomplete') } }) @@ -578,8 +616,9 @@ describe('tool validation error handling', () => { ) expect(errorEvents.length).toBe(1) expect(errorEvents[0].message).toContain( - 'tool arguments were a string, not a JSON object', + 'expected the tool arguments to be an object, but received a string', ) + expect(errorEvents[0].message).toContain('Parsing as JSON failed:') expect(errorEvents[0].message).toContain('Original tool call input:') expect(result.hadToolCallError).toBe(true) diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index de97e27bf..39161f77b 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -130,13 +130,13 @@ function stringInputError( parseError?: string, ): ToolCallError { const parseDetails = parseError - ? ` The JSON parser reported: ${parseError}. If the arguments are incomplete, re-issue the full object.` - : '' + ? ` Parsing as JSON failed: ${parseError}. The arguments may be malformed or incomplete.` + : ' Parsing succeeded, but the parsed value was still a string.' return { toolName, toolCallId, input: {}, - error: `Invalid parameters for ${toolName}: tool arguments were a string, not a JSON object. The runtime tried to parse stringified JSON before validation, but the value was still not a JSON object.${parseDetails} Re-issue the tool call as a JSON object with properly escaped string values.`, + error: `Invalid parameters for ${toolName}: expected the tool arguments to be an object, but received a string.${parseDetails} Re-issue the tool call with the full arguments object and properly escaped string values.`, } } From 6a18ebfd8feb96962727509fe939bb87460d8cd5 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 22:10:19 -0700 Subject: [PATCH 2/2] Fix duplicate reviewer agent card --- .../__tests__/message-block-helpers.test.ts | 4 + .../__tests__/sdk-event-handlers.test.ts | 83 +++++++++++++++++++ .../__tests__/send-message-helpers.test.ts | 4 + cli/src/utils/message-block-helpers.ts | 9 +- cli/src/utils/sdk-event-handlers.ts | 1 + cli/src/utils/spawn-agent-matcher.ts | 3 + .../agent-runtime/src/tools/tool-executor.ts | 15 +++- 7 files changed, 115 insertions(+), 4 deletions(-) diff --git a/cli/src/utils/__tests__/message-block-helpers.test.ts b/cli/src/utils/__tests__/message-block-helpers.test.ts index 304514aab..d813de400 100644 --- a/cli/src/utils/__tests__/message-block-helpers.test.ts +++ b/cli/src/utils/__tests__/message-block-helpers.test.ts @@ -39,6 +39,10 @@ describe('getAgentBaseName', () => { expect(getAgentBaseName('file-picker')).toBe('file-picker') }) + test('normalizes direct tool aliases to canonical agent names', () => { + expect(getAgentBaseName('code_reviewer_lite')).toBe('code-reviewer-lite') + }) + test('handles scoped name without version', () => { expect(getAgentBaseName('codebuff/file-picker')).toBe('file-picker') }) diff --git a/cli/src/utils/__tests__/sdk-event-handlers.test.ts b/cli/src/utils/__tests__/sdk-event-handlers.test.ts index 051a59689..b86566b43 100644 --- a/cli/src/utils/__tests__/sdk-event-handlers.test.ts +++ b/cli/src/utils/__tests__/sdk-event-handlers.test.ts @@ -212,6 +212,89 @@ describe('sdk-event-handlers', () => { expect(getStreamingAgents().has('tool-1-0')).toBe(false) }) + test('matches underscore direct-tool aliases to hyphenated agent ids', () => { + const { ctx, getMessages, getStreamingAgents } = createTestContext() + const handleEvent = createEventHandler(ctx) + const handleChunk = createStreamChunkHandler(ctx) + + handleEvent({ + type: 'tool_call', + toolCallId: 'tool-1', + toolName: 'spawn_agents', + input: { + agents: [ + { + agent_type: 'code_reviewer_lite', + prompt: 'Review this change', + }, + ], + }, + agentId: 'main-agent', + parentAgentId: undefined, + } as any) + + handleEvent({ + type: 'subagent_start', + agentId: 'agent-real', + agentType: 'code-reviewer-lite', + displayName: 'Code Reviewer Lite', + onlyChild: true, + parentAgentId: undefined, + params: undefined, + prompt: 'Review this change', + }) + + handleChunk({ + type: 'subagent_chunk', + agentId: 'agent-real', + agentType: 'code-reviewer-lite', + chunk: 'streamed review', + }) + + handleEvent({ + type: 'subagent_finish', + agentId: 'agent-real', + agentType: 'code-reviewer-lite', + displayName: 'Code Reviewer Lite', + onlyChild: true, + parentAgentId: undefined, + params: undefined, + prompt: 'Review this change', + }) + + handleEvent({ + type: 'tool_result', + toolCallId: 'tool-1', + toolName: 'spawn_agents', + output: [ + { + type: 'json', + value: [ + { + agentName: 'code-reviewer-lite', + agentType: 'code-reviewer-lite', + value: 'streamed review', + }, + ], + }, + ], + } as any) + + const blocks = getMessages()[0].blocks ?? [] + expect(blocks).toHaveLength(1) + const agentBlock = blocks[0] as AgentContentBlock + expect(agentBlock.agentId).toBe('agent-real') + expect(agentBlock.agentName).toBe('code-reviewer-lite') + expect(agentBlock.agentType).toBe('code-reviewer-lite') + expect(agentBlock.status).toBe('complete') + expect(agentBlock.blocks).toHaveLength(1) + expect(agentBlock.blocks?.[0]).toMatchObject({ + type: 'text', + content: 'streamed review', + }) + expect(getStreamingAgents().size).toBe(0) + }) + test('handles spawn_agents tool results and clears streaming agents', () => { const { ctx, getMessages, getStreamingAgents } = createTestContext() ctx.message.updater.addBlock( diff --git a/cli/src/utils/__tests__/send-message-helpers.test.ts b/cli/src/utils/__tests__/send-message-helpers.test.ts index 4967498cf..00f95b899 100644 --- a/cli/src/utils/__tests__/send-message-helpers.test.ts +++ b/cli/src/utils/__tests__/send-message-helpers.test.ts @@ -1325,6 +1325,10 @@ describe('getAgentBaseName', () => { test('returns simple name unchanged', () => { expect(getAgentBaseName('file-picker')).toBe('file-picker') }) + + test('normalizes direct tool aliases to canonical agent names', () => { + expect(getAgentBaseName('code_reviewer_lite')).toBe('code-reviewer-lite') + }) }) describe('agentTypesMatch', () => { diff --git a/cli/src/utils/message-block-helpers.ts b/cli/src/utils/message-block-helpers.ts index b9668da41..2d0eb29fe 100644 --- a/cli/src/utils/message-block-helpers.ts +++ b/cli/src/utils/message-block-helpers.ts @@ -16,10 +16,11 @@ import type { * getAgentBaseName('codebuff/file-picker@0.0.2') // 'file-picker' * getAgentBaseName('file-picker@1.0.0') // 'file-picker' * getAgentBaseName('file-picker') // 'file-picker' + * getAgentBaseName('file_picker') // 'file-picker' */ export const getAgentBaseName = (type: string): string => { const segment = type.split('/').pop() ?? type - return segment.split('@')[0] + return segment.split('@')[0].replace(/_/g, '-') } /** @@ -466,6 +467,7 @@ export const moveSpawnAgentBlock = ( parentId?: string, params?: Record, prompt?: string, + realAgentType?: string, ): ContentBlock[] => { const updateAgentBlock = (block: ContentBlock): ContentBlock => { if (block.type !== 'agent') { @@ -484,6 +486,11 @@ export const moveSpawnAgentBlock = ( updatedBlock.initialPrompt = prompt } + if (realAgentType) { + updatedBlock.agentType = realAgentType + updatedBlock.agentName = realAgentType + } + return updatedBlock } diff --git a/cli/src/utils/sdk-event-handlers.ts b/cli/src/utils/sdk-event-handlers.ts index 6f304f147..42c273a82 100644 --- a/cli/src/utils/sdk-event-handlers.ts +++ b/cli/src/utils/sdk-event-handlers.ts @@ -183,6 +183,7 @@ const handleSubagentStart = ( blocks, match: spawnAgentMatch, realAgentId: event.agentId, + realAgentType: event.agentType, parentAgentId: event.parentAgentId, params: event.params, prompt: event.prompt, diff --git a/cli/src/utils/spawn-agent-matcher.ts b/cli/src/utils/spawn-agent-matcher.ts index c3eb5c054..a87e493b1 100644 --- a/cli/src/utils/spawn-agent-matcher.ts +++ b/cli/src/utils/spawn-agent-matcher.ts @@ -28,6 +28,7 @@ export const resolveSpawnAgentToReal = (options: { blocks: ContentBlock[] match: SpawnAgentMatch realAgentId: string + realAgentType?: string parentAgentId?: string params?: Record prompt?: string @@ -36,6 +37,7 @@ export const resolveSpawnAgentToReal = (options: { blocks, match, realAgentId, + realAgentType, parentAgentId, params: agentParams, prompt, @@ -48,5 +50,6 @@ export const resolveSpawnAgentToReal = (options: { parentAgentId, agentParams, prompt, + realAgentType, ) } diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 39161f77b..f50e8823c 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -1,5 +1,6 @@ import { endsAgentStepParam, toolNames } from '@codebuff/common/tools/constants' import { toolParams } from '@codebuff/common/tools/list' +import { normalizeAgentIdForLookup } from '@codebuff/common/util/agent-id-parsing' import { cloneDeep } from 'lodash' import { getMCPToolData } from '../mcp' @@ -371,7 +372,9 @@ export async function executeToolCall( } } - let agentIdToLoad = agentTypeStr + let agentIdToLoad = isBaseAgent + ? normalizeAgentIdForLookup(agentTypeStr) + : agentTypeStr if (!isBaseAgent) { const matchingSpawn = getMatchingSpawn( agentTemplate.spawnableAgents, @@ -420,7 +423,13 @@ export async function executeToolCall( } } - return { valid: true as const, agent } + return { + valid: true as const, + agent: { + ...(agent as Record), + agent_type: agentIdToLoad, + }, + } }), ) @@ -449,8 +458,8 @@ export async function executeToolCall( } const errorMsg = `Some agents could not be spawned: ${errors.join('; ')}. Proceeding with valid agents only.` onResponseChunk({ type: 'error', message: errorMsg }) - effectiveInput = { ...effectiveInput, agents: validAgents } } + effectiveInput = { ...effectiveInput, agents: validAgents } } }