Skip to content

Conversation

@MartkCz
Copy link

@MartkCz MartkCz commented Sep 17, 2025

Motivation and Context

The current tool execution system lacks a standardized way for handling tool-specific errors. This change provides a consistent way for tools to report errors.

How Has This Been Tested?

Added unit tests in ToolCallerTest.php

Breaking Changes

No breaking changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

The implementation includes modifications to:

  • ToolCaller and ToolCallerInterface for exception handling integration
  • ToolChain for proper error propagation
  • New ToolExecutionExceptionInterface defining the contract for custom tool error handling

Usage

class McpElementsException extends RuntimException implements ToolExecutionExceptionInterface { ... }

class McpElements
{
	#[McpTool(name: 'calculate')]
	public function calculate(float $a, float $b, string $operation): float|string
	{
		$op = strtolower($operation);

		switch ($op) {
			case 'add':
				$result = $a + $b;
				break;
			case 'subtract':
				$result = $a - $b;
				break;
			case 'multiply':
				$result = $a * $b;
				break;
			case 'divide':
				if (0 == $b) {
					throw new McpElementsException(['Division by zero.']);
				}
				$result = $a / $b;
				break;
			default:
				throw new McpElementsException(["Unknown operation '{$operation}'. Supported: add, subtract, multiply, divide."]);
		}

		if (!$this->config['allow_negative'] && $result < 0) {
			throw new McpElementsException(['Negative results are disabled.']);
		}

		return round($result, $this->config['precision']);
	}
}

@chr-hertel
Copy link
Member

Hi @MartkCz,
I think you're right, we should at least document how users should deal with that kind of situation, but I disagree on the naming here.

The ReferenceHandlerInterface is rather generic and does/should not care/know about the concrete element implementation of tools or prompts, etc. In your example you're implementing the exception even in a generic way again with an McpElementsException - a bit confusing tbh.

Currently the ReferenceHandlerInterface states already, that you should throw a RegistryException "if execution fails". So the use case is covered, only the exception is not tailored.

I would prefer, instead of creating a new interface to implement, you create a ReferenceExecutionException extends RegistryException or ElementsExecutionException or similar - which is more generic than tool specific - and that new one replacing the RegistryException on the interface. WDYT?

@chr-hertel chr-hertel changed the title Add ToolExecutionExceptionInterface for custom tool error handling [Server] Add ToolExecutionExceptionInterface for custom tool error handling Sep 21, 2025
@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Sep 21, 2025
@MartkCz
Copy link
Author

MartkCz commented Sep 22, 2025

@chr-hertel Thanks for your time.

  1. I removed duplicate logging in the CallToolHandler (all logging is already here: https://github.com/modelcontextprotocol/php-sdk/blob/main/src/Capability/Tool/ToolCaller.php#L51-L76
    ).
  2. I fixed https://github.com/modelcontextprotocol/php-sdk/blob/main/tests/Server/RequestHandler/CallToolHandlerTest.php#L117-L145
    — when a tool is not found, the code should not be -32603 (INTERNAL_ERROR), but -32602 (INVALID_PARAMS), see https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling
    .
  3. I think it creates a mess because RegistryException and ReferenceExecutionException must be wrapped into ToolCallException and then unwrapped to determine whether it’s a protocol error or an execution error, since each is handled differently. On top of that, there’s a separate protocol error ToolNotFoundException. In my opinion, the best approach would be to split them into protocol and execution exceptions and throw those instead of ToolCallException and ToolNotFoundException.
interface ReferenceExceptionInterface {}

final class ExecutionReferenceException extends \RuntimException implements ReferenceExceptionInterface {} // business errors

abstract class ProtocolReferenceException extends \LogicException implements ReferenceExceptionInterface {} // unknown tools, invalid arguments, server errors

final class ToolNotFoundReferenceException extends ProtocolReferenceException {}

final class InvalidParamsReferenceException extends ProtocolReferenceException {}

final class InternalErrorReferenceException extends ProtocolReferenceException {}

This is just a proposal — there wouldn’t necessarily have to be so many exception classes inheriting from ProtocolReferenceException, but I wanted to preserve throwing ToolNotFoundException. The naming is also not final.

and then this is simplified from this:

try {
   $result = $this->referenceHandler->handle($toolReference, $arguments);
   /** @var TextContent[]|ImageContent[]|EmbeddedResource[]|AudioContent[] $formattedResult */
   $formattedResult = $toolReference->formatResult($result);

   $this->logger->debug('Tool executed successfully', [
       'name' => $toolName,
       'result_type' => \gettype($result),
   ]);

   return new CallToolResult($formattedResult);
} catch (RegistryException $e) {
   throw new ToolCallException($request, $e);
} catch (\Throwable $e) {
   if ($e instanceof ToolCallException) {
       throw $e;
   }

   $this->logger->error('Tool execution failed', [
       'name' => $toolName,
       'exception' => $e->getMessage(),
       'trace' => $e->getTraceAsString(),
   ]);

   throw new ToolCallException($request, RegistryException::internalError('Error while executing tool', $e));
}

to this:

try {
	$result = $this->referenceHandler->handle($toolReference, $arguments);
	/** @var TextContent[]|ImageContent[]|EmbeddedResource[]|AudioContent[] $formattedResult */
	$formattedResult = $toolReference->formatResult($result);

	$this->logger->debug('Tool executed successfully', [
		'name' => $toolName,
		'result_type' => \gettype($result),
	]);

	return new CallToolResult($formattedResult);
} catch (ReferenceExceptionInterface $e) {
	throw $e;
} catch (\Throwable $e) {
	$this->logger->error('Tool execution failed', [
		'name' => $toolName,
		'exception' => $e->getMessage(),
		'trace' => $e->getTraceAsString(),
	]);

	throw new InternalErrorReferenceException($request, $e);
}

and this:

try {
    $content = $this->toolCaller->call($message);
} catch (ToolNotFoundException $exception) {
    return Error::forInvalidParams($exception->getMessage(), $message->getId());
} catch (ToolCallException $exception) {
    $registryException = $exception->registryException;

    if ($registryException instanceof ReferenceExecutionException) {
        return new Response($message->getId(), CallToolResult::error(array_map(
            fn (string $message): TextContent => new TextContent($message),
            $registryException->messages,
        )));
    }

		return new Error($message->getId(), $registryException->getCode(), $registryException->getMessage());
}

to this:

try {
	$content = $this->toolCaller->call($message);
} catch (ProtocolReferenceException $exception) {
	return new Error($message->getId(), $exception->getCode(), $exception->getMessage());
} catch (ExecutionReferenceException $exception) {
	return new Response($message->getId(), CallToolResult::error(array_map(
		fn (string $message): TextContent => new TextContent($message),
		$exception->messages,
	)));
}

@chr-hertel
Copy link
Member

@MartkCz Thanks for the follow-up and the detailed explanation! 🙏
I think your proposal sounds & looks reasonable to me 👍

btw, this needs a rebase - and please make sure to have a squashed, signed commit please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants