refactor(mcp): Introduce buildMcpToolSuccessResponse and buildMcpToolErrorResponse for consistent response handling

This commit is contained in:
Kazuki Yamada
2025-05-25 16:51:09 +09:00
parent ec413eb760
commit 9f6aa1ad10
11 changed files with 227 additions and 264 deletions

View File

@@ -4,6 +4,7 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
import { z } from 'zod';
import { logger } from '../../shared/logger.js';
import { buildMcpToolErrorResponse, buildMcpToolSuccessResponse } from './mcpToolRuntime.js';
/**
* Register file system directory listing tool
@@ -28,41 +29,19 @@ export const registerFileSystemReadDirectoryTool = (mcpServer: McpServer) => {
// Ensure path is absolute
if (!path.isAbsolute(directoryPath)) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Path must be absolute. Received: ${directoryPath}`,
},
],
};
return buildMcpToolErrorResponse([`Error: Path must be absolute. Received: ${directoryPath}`]);
}
// Check if directory exists
try {
const stats = await fs.stat(directoryPath);
if (!stats.isDirectory()) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: The specified path is not a directory: ${directoryPath}. Use file_system_read_file for files.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: The specified path is not a directory: ${directoryPath}. Use file_system_read_file for files.`,
]);
}
} catch {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Directory not found at path: ${directoryPath}`,
},
],
};
return buildMcpToolErrorResponse([`Error: Directory not found at path: ${directoryPath}`]);
}
// Read directory contents
@@ -71,29 +50,12 @@ export const registerFileSystemReadDirectoryTool = (mcpServer: McpServer) => {
.map((entry) => `${entry.isDirectory() ? '[DIR]' : '[FILE]'} ${entry.name}`)
.join('\n');
return {
content: [
{
type: 'text',
text: `Contents of ${directoryPath}:`,
},
{
type: 'text',
text: formatted || '(empty directory)',
},
],
};
return buildMcpToolSuccessResponse([`Contents of ${directoryPath}:`, formatted || '(empty directory)']);
} catch (error) {
logger.error(`Error in file_system_read_directory tool: ${error}`);
return {
isError: true,
content: [
{
type: 'text',
text: `Error listing directory: ${error instanceof Error ? error.message : String(error)}`,
},
],
};
return buildMcpToolErrorResponse([
`Error listing directory: ${error instanceof Error ? error.message : String(error)}`,
]);
}
},
);

View File

@@ -6,6 +6,7 @@ import { z } from 'zod';
import type { SuspiciousFileResult } from '../../core/security/securityCheck.js';
import { createSecretLintConfig, runSecretLint } from '../../core/security/workers/securityCheckWorker.js';
import { logger } from '../../shared/logger.js';
import { buildMcpToolErrorResponse, buildMcpToolSuccessResponse } from './mcpToolRuntime.js';
/**
* Register file system read file tool with security checks
@@ -30,44 +31,22 @@ export const registerFileSystemReadFileTool = (mcpServer: McpServer) => {
// Ensure path is absolute
if (!path.isAbsolute(filePath)) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Path must be absolute. Received: ${filePath}`,
},
],
};
return buildMcpToolErrorResponse([`Error: Path must be absolute. Received: ${filePath}`]);
}
// Check if file exists
try {
await fs.access(filePath);
} catch {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: File not found at path: ${filePath}`,
},
],
};
return buildMcpToolErrorResponse([`Error: File not found at path: ${filePath}`]);
}
// Check if it's a directory
const stats = await fs.stat(filePath);
if (stats.isDirectory()) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: The specified path is a directory, not a file: ${filePath}. Use file_system_read_directory for directories.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: The specified path is a directory, not a file: ${filePath}. Use file_system_read_directory for directories.`,
]);
}
// Read file content
@@ -79,40 +58,17 @@ export const registerFileSystemReadFileTool = (mcpServer: McpServer) => {
// If security check found issues, block the file
if (securityCheckResult !== null) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Security check failed. The file at ${filePath} may contain sensitive information.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Security check failed. The file at ${filePath} may contain sensitive information.`,
]);
}
return {
content: [
{
type: 'text',
text: `Content of ${filePath}:`,
},
{
type: 'text',
text: fileContent,
},
],
};
return buildMcpToolSuccessResponse([`Content of ${filePath}:`, fileContent]);
} catch (error) {
logger.error(`Error in file_system_read_file tool: ${error}`);
return {
isError: true,
content: [
{
type: 'text',
text: `Error reading file: ${error instanceof Error ? error.message : String(error)}`,
},
],
};
return buildMcpToolErrorResponse([
`Error reading file: ${error instanceof Error ? error.message : String(error)}`,
]);
}
},
);

View File

@@ -3,7 +3,7 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
import { z } from 'zod';
import { logger } from '../../shared/logger.js';
import { getOutputFilePath } from './mcpToolRuntime.js';
import { buildMcpToolErrorResponse, buildMcpToolSuccessResponse, getOutputFilePath } from './mcpToolRuntime.js';
/**
* Search options for grep functionality
@@ -83,29 +83,17 @@ export const registerGrepRepomixOutputTool = (mcpServer: McpServer) => {
const filePath = getOutputFilePath(outputId);
if (!filePath) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Output file with ID ${outputId} not found. The output file may have been deleted or the ID is invalid.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Output file with ID ${outputId} not found. The output file may have been deleted or the ID is invalid.`,
]);
}
try {
await fs.access(filePath);
} catch (error) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Output file does not exist at path: ${filePath}. The temporary file may have been cleaned up.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Output file does not exist at path: ${filePath}. The temporary file may have been cleaned up.`,
]);
}
const content = await fs.readFile(filePath, 'utf8');
@@ -125,51 +113,24 @@ export const registerGrepRepomixOutputTool = (mcpServer: McpServer) => {
ignoreCase,
});
} catch (error) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: ${error instanceof Error ? error.message : String(error)}`,
},
],
};
return buildMcpToolErrorResponse([`Error: ${error instanceof Error ? error.message : String(error)}`]);
}
if (searchResult.matches.length === 0) {
return {
content: [
{
type: 'text',
text: `No matches found for pattern "${pattern}" in Repomix output file (ID: ${outputId}).`,
},
],
};
return buildMcpToolSuccessResponse([
`No matches found for pattern "${pattern}" in Repomix output file (ID: ${outputId}).`,
]);
}
return {
content: [
{
type: 'text',
text: `Found ${searchResult.matches.length} match(es) for pattern "${pattern}" in Repomix output file (ID: ${outputId}):`,
},
{
type: 'text',
text: searchResult.formattedOutput.join('\n'),
},
],
};
return buildMcpToolSuccessResponse([
`Found ${searchResult.matches.length} match(es) for pattern "${pattern}" in Repomix output file (ID: ${outputId}):`,
searchResult.formattedOutput.join('\n'),
]);
} catch (error) {
logger.error(`Error in grep_repomix_output: ${error}`);
return {
isError: true,
content: [
{
type: 'text',
text: `Error searching Repomix output: ${error instanceof Error ? error.message : String(error)}`,
},
],
};
return buildMcpToolErrorResponse([
`Error searching Repomix output: ${error instanceof Error ? error.message : String(error)}`,
]);
}
},
);

View File

@@ -190,3 +190,28 @@ export const formatToolError = (error: unknown): CallToolResult => {
],
};
};
/**
* Creates a successful MCP tool response with type safety
*/
export const buildMcpToolSuccessResponse = (messages: string[]): CallToolResult => {
return {
content: messages.map((message) => ({
type: 'text' as const,
text: message,
})),
};
};
/**
* Creates an error MCP tool response with type safety
*/
export const buildMcpToolErrorResponse = (errorMessages: string[]): CallToolResult => {
return {
isError: true,
content: errorMessages.map((message) => ({
type: 'text' as const,
text: message,
})),
};
};

View File

@@ -4,7 +4,12 @@ import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
import { z } from 'zod';
import { runCli } from '../../cli/cliRun.js';
import type { CliOptions } from '../../cli/types.js';
import { createToolWorkspace, formatToolError, formatToolResponse } from './mcpToolRuntime.js';
import {
buildMcpToolErrorResponse,
createToolWorkspace,
formatToolError,
formatToolResponse,
} from './mcpToolRuntime.js';
export const registerPackCodebaseTool = (mcpServer: McpServer) => {
mcpServer.tool(
@@ -65,22 +70,7 @@ export const registerPackCodebaseTool = (mcpServer: McpServer) => {
const result = await runCli(['.'], directory, cliOptions);
if (!result) {
return {
isError: true,
content: [
{
type: 'text',
text: JSON.stringify(
{
success: false,
error: 'Failed to return a result',
},
null,
2,
),
},
],
};
return buildMcpToolErrorResponse(['Failed to return a result']);
}
// Extract metrics information from the pack result

View File

@@ -4,7 +4,12 @@ import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
import { z } from 'zod';
import { runCli } from '../../cli/cliRun.js';
import type { CliOptions } from '../../cli/types.js';
import { createToolWorkspace, formatToolError, formatToolResponse } from './mcpToolRuntime.js';
import {
buildMcpToolErrorResponse,
createToolWorkspace,
formatToolError,
formatToolResponse,
} from './mcpToolRuntime.js';
export const registerPackRemoteRepositoryTool = (mcpServer: McpServer) => {
mcpServer.tool(
@@ -70,15 +75,7 @@ export const registerPackRemoteRepositoryTool = (mcpServer: McpServer) => {
const result = await runCli(['.'], process.cwd(), cliOptions);
if (!result) {
return {
isError: true,
content: [
{
type: 'text',
text: 'Failed to return a result',
},
],
};
return buildMcpToolErrorResponse(['Failed to return a result']);
}
// Extract metrics information from the pack result

View File

@@ -3,7 +3,7 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';
import { z } from 'zod';
import { logger } from '../../shared/logger.js';
import { getOutputFilePath } from './mcpToolRuntime.js';
import { buildMcpToolErrorResponse, buildMcpToolSuccessResponse, getOutputFilePath } from './mcpToolRuntime.js';
/**
* Register the tool to read Repomix output files
@@ -37,30 +37,18 @@ export const registerReadRepomixOutputTool = (mcpServer: McpServer) => {
// Get the file path from the registry
const filePath = getOutputFilePath(outputId);
if (!filePath) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Output file with ID ${outputId} not found. The output file may have been deleted or the ID is invalid.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Output file with ID ${outputId} not found. The output file may have been deleted or the ID is invalid.`,
]);
}
// Check if the file exists
try {
await fs.access(filePath);
} catch (error) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Output file does not exist at path: ${filePath}. The temporary file may have been cleaned up.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Output file does not exist at path: ${filePath}. The temporary file may have been cleaned up.`,
]);
}
// Read the file content
@@ -70,15 +58,9 @@ export const registerReadRepomixOutputTool = (mcpServer: McpServer) => {
if (startLine !== undefined || endLine !== undefined) {
// Validate that startLine is less than or equal to endLine when both are provided
if (startLine !== undefined && endLine !== undefined && startLine > endLine) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Start line (${startLine}) cannot be greater than end line (${endLine}).`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Start line (${startLine}) cannot be greater than end line (${endLine}).`,
]);
}
const lines = content.split('\n');
@@ -86,43 +68,23 @@ export const registerReadRepomixOutputTool = (mcpServer: McpServer) => {
const end = endLine ? Math.min(lines.length, endLine) : lines.length;
if (start >= lines.length) {
return {
isError: true,
content: [
{
type: 'text',
text: `Error: Start line ${startLine} exceeds total lines (${lines.length}) in the file.`,
},
],
};
return buildMcpToolErrorResponse([
`Error: Start line ${startLine} exceeds total lines (${lines.length}) in the file.`,
]);
}
processedContent = lines.slice(start, end).join('\n');
}
return {
content: [
{
type: 'text',
text: `Content of Repomix output file (ID: ${outputId})${startLine || endLine ? ` (lines ${startLine || 1}-${endLine || 'end'})` : ''}:`,
},
{
type: 'text',
text: processedContent,
},
],
};
return buildMcpToolSuccessResponse([
`Content of Repomix output file (ID: ${outputId})${startLine || endLine ? ` (lines ${startLine || 1}-${endLine || 'end'})` : ''}:`,
processedContent,
]);
} catch (error) {
logger.error(`Error reading Repomix output: ${error}`);
return {
isError: true,
content: [
{
type: 'text',
text: `Error reading Repomix output: ${error instanceof Error ? error.message : String(error)}`,
},
],
};
return buildMcpToolErrorResponse([
`Error reading Repomix output: ${error instanceof Error ? error.message : String(error)}`,
]);
}
},
);

View File

@@ -11,13 +11,14 @@ import {
import * as mcpToolRuntime from '../../../src/mcp/tools/mcpToolRuntime.js';
vi.mock('node:fs/promises');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js');
vi.mock('../../../src/shared/logger.js', () => ({
logger: {
trace: vi.fn(),
error: vi.fn(),
},
}));
vi.mock('../../../src/shared/logger.js');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js', async () => {
const actual = await vi.importActual('../../../src/mcp/tools/mcpToolRuntime.js');
return {
...actual,
getOutputFilePath: vi.fn(),
};
});
/**
* Search options for grep functionality

View File

@@ -4,6 +4,8 @@ import os from 'node:os';
import path from 'node:path';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import {
buildMcpToolErrorResponse,
buildMcpToolSuccessResponse,
createToolWorkspace,
formatToolError,
formatToolResponse,
@@ -188,4 +190,104 @@ describe('mcpToolRuntime', () => {
expect(jsonContent.metrics.totalLines).toBe(5);
});
});
describe('buildMcpToolSuccessResponse', () => {
it('should create a successful response with single message', () => {
const messages = ['Operation completed successfully'];
const response = buildMcpToolSuccessResponse(messages);
expect(response).toEqual({
content: [
{
type: 'text',
text: 'Operation completed successfully',
},
],
});
expect(response.isError).toBeUndefined();
});
it('should create a successful response with multiple messages', () => {
const messages = ['First message', 'Second message', 'Third message'];
const response = buildMcpToolSuccessResponse(messages);
expect(response).toEqual({
content: [
{
type: 'text',
text: 'First message',
},
{
type: 'text',
text: 'Second message',
},
{
type: 'text',
text: 'Third message',
},
],
});
expect(response.isError).toBeUndefined();
});
it('should create a successful response with empty messages array', () => {
const messages: string[] = [];
const response = buildMcpToolSuccessResponse(messages);
expect(response).toEqual({
content: [],
});
expect(response.isError).toBeUndefined();
});
});
describe('buildMcpToolErrorResponse', () => {
it('should create an error response with single message', () => {
const errorMessages = ['Something went wrong'];
const response = buildMcpToolErrorResponse(errorMessages);
expect(response).toEqual({
isError: true,
content: [
{
type: 'text',
text: 'Something went wrong',
},
],
});
});
it('should create an error response with multiple messages', () => {
const errorMessages = ['Error 1', 'Error 2', 'Error 3'];
const response = buildMcpToolErrorResponse(errorMessages);
expect(response).toEqual({
isError: true,
content: [
{
type: 'text',
text: 'Error 1',
},
{
type: 'text',
text: 'Error 2',
},
{
type: 'text',
text: 'Error 3',
},
],
});
});
it('should create an error response with empty messages array', () => {
const errorMessages: string[] = [];
const response = buildMcpToolErrorResponse(errorMessages);
expect(response).toEqual({
isError: true,
content: [],
});
});
});
});

View File

@@ -8,7 +8,15 @@ import { registerPackCodebaseTool } from '../../../src/mcp/tools/packCodebaseToo
vi.mock('node:path');
vi.mock('../../../src/cli/cliRun.js');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js', async () => {
const actual = await vi.importActual('../../../src/mcp/tools/mcpToolRuntime.js');
return {
...actual,
createToolWorkspace: vi.fn(),
formatToolError: vi.fn(),
formatToolResponse: vi.fn(),
};
});
describe('PackCodebaseTool', () => {
const mockServer = {
@@ -144,14 +152,7 @@ describe('PackCodebaseTool', () => {
content: [
{
type: 'text',
text: JSON.stringify(
{
success: false,
error: 'Failed to return a result',
},
null,
2,
),
text: 'Failed to return a result',
},
],
});

View File

@@ -5,7 +5,13 @@ import * as mcpToolRuntime from '../../../src/mcp/tools/mcpToolRuntime.js';
import { registerReadRepomixOutputTool } from '../../../src/mcp/tools/readRepomixOutputTool.js';
vi.mock('node:fs/promises');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js');
vi.mock('../../../src/mcp/tools/mcpToolRuntime.js', async () => {
const actual = await vi.importActual('../../../src/mcp/tools/mcpToolRuntime.js');
return {
...actual,
getOutputFilePath: vi.fn(),
};
});
vi.mock('../../../src/shared/logger.js', () => ({
logger: {
trace: vi.fn(),