Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions PR_DESCRIPTION_2020.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
## Problem

When reading PDF files, the API returns "Invalid value: file. Supported values are: 'text','image_url','video_url' and 'video'." error. Worse, this error state persists in the session, causing all subsequent requests to fail with the same error.

Fixes #2020

## Root Cause

Gemini API's FunctionResponse does not support PDF (`application/pdf`) in the `parts` field. When a tool returns PDF content as inlineData, it gets passed through to the API, which rejects it.

## Changes

- Add PDF (`application/pdf`) to unsupported media types in `convertUnsupportedMediaToText()`
- PDF content in tool responses is now converted to explanatory text instead of being sent as inlineData
- This prevents the API error and allows the session to continue normally

## Testing

- Added test case for PDF inlineData conversion
- Added test case for PDF fileData conversion
- All 11 tests in geminiContentGenerator.test.ts pass
104 changes: 104 additions & 0 deletions packages/core/src/core/coreToolScheduler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,110 @@ describe('CoreToolScheduler', () => {
expect(errorMessage).not.toContain('requires permission');
}
});

it('should migrate legacy tool names like "bash" to "run_shell_command" (fix for #2012)', async () => {
const onAllToolCallsComplete = vi.fn();
const onToolCallsUpdate = vi.fn();

// Track if execute was called
let executeCalled = false;

// Mock shell tool that should be called when using "bash" alias
const mockShellTool = new MockTool({
name: 'run_shell_command',
displayName: 'Shell',
description: 'Run shell commands',
execute: async () => {
executeCalled = true;
return {
llmContent: 'Command executed successfully',
returnDisplay: 'Executed',
};
},
});

const mockToolRegistry = {
getTool: (name: string) => {
// Should receive migrated name "run_shell_command" not "bash"
if (name === 'run_shell_command') {
return mockShellTool;
}
return undefined;
},
getAllToolNames: () => ['run_shell_command', 'read_file'],
getFunctionDeclarations: () => [],
tools: new Map(),
discovery: {},
registerTool: () => {},
getToolByName: () => undefined,
getToolByDisplayName: () => undefined,
getTools: () => [],
discoverTools: async () => {},
getAllTools: () => [],
getToolsByServer: () => [],
} as unknown as ToolRegistry;

const mockConfig = {
getSessionId: () => 'test-session-id',
getUsageStatisticsEnabled: () => true,
getDebugMode: () => false,
getApprovalMode: () => ApprovalMode.YOLO, // YOLO mode to auto-approve
getAllowedTools: () => [],
getExcludeTools: () => [],
getContentGeneratorConfig: () => ({
model: 'test-model',
authType: 'gemini',
}),
getShellExecutionConfig: () => ({
terminalWidth: 90,
terminalHeight: 30,
}),
storage: {
getProjectTempDir: () => '/tmp',
},
getTruncateToolOutputThreshold: () =>
DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD,
getTruncateToolOutputLines: () => DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES,
getToolRegistry: () => mockToolRegistry,
getUseModelRouter: () => false,
getGeminiClient: () => null,
getChatRecordingService: () => undefined,
} as unknown as Config;

const scheduler = new CoreToolScheduler({
config: mockConfig,
onAllToolCallsComplete,
onToolCallsUpdate,
getPreferredEditor: () => 'vscode',
onEditorClose: vi.fn(),
});

const abortController = new AbortController();
const request = {
callId: '1',
name: 'bash', // Legacy name that should be migrated to "run_shell_command"
args: { command: 'echo hello', description: 'Test command' },
isClientInitiated: false,
prompt_id: 'prompt-id-bash',
};

await scheduler.schedule([request], abortController.signal);

// Wait for completion
await vi.waitFor(() => {
expect(onAllToolCallsComplete).toHaveBeenCalled();
});

const completedCalls = onAllToolCallsComplete.mock
.calls[0][0] as ToolCall[];
expect(completedCalls).toHaveLength(1);
const completedCall = completedCalls[0];

// Should succeed, not error
expect(completedCall.status).toBe('success');
// The shell tool should have been executed
expect(executeCalled).toBe(true);
});
});
});

Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import type {
Part,
PartListUnion,
} from '@google/genai';
import { ToolNames } from '../tools/tool-names.js';
import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js';
import { getResponseTextFromParts } from '../utils/generateContentResponseUtilities.js';
import type { ModifyContext } from '../tools/modifiable-tool.js';
import {
Expand Down Expand Up @@ -752,7 +752,13 @@ export class CoreToolScheduler {
}
}

const toolInstance = this.toolRegistry.getTool(reqInfo.name);
// Migrate legacy tool names to current names (e.g., "bash" -> "run_shell_command")
const migratedToolName =
ToolNamesMigration[
reqInfo.name as keyof typeof ToolNamesMigration
] || reqInfo.name;

const toolInstance = this.toolRegistry.getTool(migratedToolName);
if (!toolInstance) {
// Tool is not in registry and not excluded - likely hallucinated or typo
const errorMessage = this.getToolNotFoundMessage(reqInfo.name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,4 +368,46 @@ describe('GeminiContentGenerator', () => {
'Unsupported media type for Gemini: video/mp4.',
);
});

it('should convert PDF to text in functionResponse parts (fix for #2020)', async () => {
const request = {
model: 'gemini-1.5-flash',
contents: [
{
role: 'user' as const,
parts: [
{
functionResponse: {
id: 'call-1',
name: 'ReadFile',
response: { output: 'PDF content read' },
parts: [
{
inlineData: {
mimeType: 'application/pdf',
data: 'base64pdfdata',
displayName: 'document.pdf',
},
},
],
},
},
],
},
],
};

await generator.generateContent(request, 'prompt-id');

const calledWith = mockGoogleGenAI.models.generateContent.mock.calls[0][0];
const functionResponseParts =
calledWith.contents[0].parts[0].functionResponse.parts;

// PDF should be converted to text (not sent as inlineData)
expect(functionResponseParts).toHaveLength(1);
expect(functionResponseParts[0].text).toBe(
'Unsupported media type for Gemini: application/pdf (document.pdf).',
);
expect(functionResponseParts[0].inlineData).toBeUndefined();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -250,17 +250,20 @@ export class GeminiContentGenerator implements ContentGenerator {
}

/**
* Convert unsupported media types (audio, video) to explanatory text for Gemini API
* Convert unsupported media types (audio, video, pdf) to explanatory text for Gemini API.
* Note: PDF files are not supported in FunctionResponse parts and must be converted to text.
*/
private convertUnsupportedMediaToText(part: Part): Part {
if (typeof part === 'string') return part;

const inlineMimeType = part.inlineData?.mimeType || '';
const fileMimeType = part.fileData?.mimeType || '';

// Check for unsupported media types in inlineData
if (
inlineMimeType.startsWith('audio/') ||
inlineMimeType.startsWith('video/')
inlineMimeType.startsWith('video/') ||
inlineMimeType === 'application/pdf'
) {
const displayName = (part.inlineData as { displayName?: string })
?.displayName;
Expand All @@ -270,9 +273,11 @@ export class GeminiContentGenerator implements ContentGenerator {
};
}

// Check for unsupported media types in fileData
if (
fileMimeType.startsWith('audio/') ||
fileMimeType.startsWith('video/')
fileMimeType.startsWith('video/') ||
fileMimeType === 'application/pdf'
) {
const displayName = (part.fileData as { displayName?: string })
?.displayName;
Expand Down
24 changes: 10 additions & 14 deletions packages/core/src/core/openaiContentGenerator/converter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ describe('OpenAIContentConverter', () => {
expect(userMessage).toBeUndefined();
});

it('should convert PDF inlineData to tool message with embedded input_file', () => {
it('should convert PDF inlineData to text description for compatibility (fix for #2020)', () => {
const request: GenerateContentParameters = {
model: 'models/test',
contents: [
Expand Down Expand Up @@ -338,23 +338,20 @@ describe('OpenAIContentConverter', () => {

const messages = converter.convertGeminiRequestToOpenAI(request);

// Should have tool message with both text and file content
// Should have tool message with text content and PDF description
// Note: PDF is converted to text description because not all OpenAI-compatible APIs support 'file' type
const toolMessage = messages.find((message) => message.role === 'tool');
expect(toolMessage).toBeDefined();
expect(Array.isArray(toolMessage?.content)).toBe(true);
const contentArray = toolMessage?.content as Array<{
type: string;
text?: string;
file?: { filename: string; file_data: string };
}>;
expect(contentArray).toHaveLength(2);
expect(contentArray[0].type).toBe('text');
expect(contentArray[0].text).toBe('PDF content');
expect(contentArray[1].type).toBe('file');
expect(contentArray[1].file?.filename).toBe('document.pdf');
expect(contentArray[1].file?.file_data).toBe(
'data:application/pdf;base64,base64pdfdata',
);
expect(contentArray[1].type).toBe('text');
expect(contentArray[1].text).toBe('[PDF file: document.pdf (0KB)]');

// No separate user message should be created
const userMessage = messages.find((message) => message.role === 'user');
Expand Down Expand Up @@ -485,7 +482,7 @@ describe('OpenAIContentConverter', () => {
);
});

it('should convert PDF fileData URL to tool message with embedded file', () => {
it('should convert PDF fileData URL to text description for compatibility (fix for #2020)', () => {
const request: GenerateContentParameters = {
model: 'models/test',
contents: [
Expand Down Expand Up @@ -528,21 +525,20 @@ describe('OpenAIContentConverter', () => {

const messages = converter.convertGeminiRequestToOpenAI(request);

// Note: PDF is converted to text description because not all OpenAI-compatible APIs support 'file' type
const toolMessage = messages.find((message) => message.role === 'tool');
expect(toolMessage).toBeDefined();
expect(Array.isArray(toolMessage?.content)).toBe(true);
const contentArray = toolMessage?.content as Array<{
type: string;
text?: string;
file?: { filename: string; file_data: string };
}>;
expect(contentArray).toHaveLength(2);
expect(contentArray[0].type).toBe('text');
expect(contentArray[0].text).toBe('PDF content');
expect(contentArray[1].type).toBe('file');
expect(contentArray[1].file?.filename).toBe('document.pdf');
expect(contentArray[1].file?.file_data).toBe(
'https://assets.anthropic.com/m/1cd9d098ac3e6467/original/Claude-3-Model-Card-October-Addendum.pdf',
expect(contentArray[1].type).toBe('text');
expect(contentArray[1].text).toBe(
'[PDF file: document.pdf (https://assets.anthropic.com/m/1cd9d098ac3e6467/original/Claude-3-Model-Card-October-Addendum.pdf)]',
);
});

Expand Down
21 changes: 11 additions & 10 deletions packages/core/src/core/openaiContentGenerator/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,13 +600,15 @@ export class OpenAIContentConverter {
}

if (mimeType === 'application/pdf') {
// Note: Not all OpenAI-compatible APIs support 'file' type.
// Convert PDF to a text description to ensure compatibility across providers.
// This prevents "Invalid value: file" errors that can corrupt the session.
const filename = part.inlineData.displayName || 'document.pdf';
const dataSize = Math.round((part.inlineData.data.length * 3) / 4); // Approximate base64 decoded size
const sizeInKB = Math.round(dataSize / 1024);
return {
type: 'file' as const,
file: {
filename,
file_data: `data:${mimeType};base64,${part.inlineData.data}`,
},
type: 'text' as const,
text: `[PDF file: ${filename} (${sizeInKB}KB)]`,
};
}

Expand Down Expand Up @@ -655,12 +657,11 @@ export class OpenAIContentConverter {
}

if (mimeType === 'application/pdf') {
// Note: Not all OpenAI-compatible APIs support 'file' type.
// Convert PDF to a text description to ensure compatibility across providers.
return {
type: 'file' as const,
file: {
filename,
file_data: fileUri,
},
type: 'text' as const,
text: `[PDF file: ${filename} (${fileUri})]`,
};
}

Expand Down
23 changes: 23 additions & 0 deletions packages/core/src/tools/skill.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,29 @@ describe('SkillTool', () => {
);
});

it('should trim whitespace from skill name (fix for #2025)', () => {
// Test leading space
const result1 = skillTool.validateToolParams({ skill: ' code-review' });
expect(result1).toBeNull();

// Test trailing space
const result2 = skillTool.validateToolParams({ skill: 'code-review ' });
expect(result2).toBeNull();

// Test both leading and trailing spaces
const result3 = skillTool.validateToolParams({ skill: ' code-review ' });
expect(result3).toBeNull();

// Test with Chinese skill name (as reported in issue)
const result4 = skillTool.validateToolParams({ skill: ' testing ' });
expect(result4).toBeNull();
});

it('should still reject skill with only whitespace', () => {
const result = skillTool.validateToolParams({ skill: ' ' });
expect(result).toBe('Parameter "skill" must be a non-empty string.');
});

it('should show appropriate message when no skills available', async () => {
vi.mocked(mockSkillManager.listSkills).mockResolvedValue([]);

Expand Down
Loading