Cover branches in Tool.php and fix validation logic

src/Tool/Tool.php

@@ -233,12 +233,14 @@ protected function validateArguments(array $arguments): void
         }
 
         foreach ($this->parameters as $name => $param) {
-            if ($param->required && !isset($arguments[$name])) {
+            if ($param->required && !array_key_exists($name, $arguments)) {
                 throw new \InvalidArgumentException("Missing required argument: {$name}");
             }
             // If parameter is present, validate its type.
             // If not required and not present, skip type validation.
-            if (isset($arguments[$name])) {
+            // For required parameters that are null (like a pAny = null), they should still be validated by type.
+            // So, we check if the key exists for type validation.
+            if (array_key_exists($name, $arguments)) {
                 if (!$this->validateType($arguments[$name], $param->type)) {
                     throw new \InvalidArgumentException(
                         "Invalid type for argument {$name}: expected {$param->type}, got " . gettype($arguments[$name])
@@ -255,18 +257,41 @@ protected function validateArguments(array $arguments): void
      * @param string $type The expected type.
      * @return bool True if the value is of the expected type, false otherwise.
      */
-    private function validateType($value, string $type): bool
+    private function validateType(mixed $value, string $type): bool
     {
+        // If the type is 'any', any value (including null) is acceptable.
+        if ($type === 'any') {
+            return true;
+        }
+
+        // For other types, if the value is null, it's only valid if the type explicitly allows null
+        // (e.g. future support for nullable types like "string|null" or an attribute property).
+        // For now, basic types like 'string', 'integer' do not implicitly accept null
+        // unless we define them as such (e.g. by convention or a new 'nullable' property in ParameterAttribute).
+        // However, the current problem is about 'any' type and 'required' check.
+        // The validateType method for non-'any' types should typically return false for null
+        // if the type itself isn't nullable.
+        // This part of logic might need refinement if nullable types (e.g. ?string) are formally introduced.
+
+        // If value is null and type is not 'any', it's an invalid type for basic scalar types by default.
+        // This strictness might be too much if a required parameter of type 'string' can be null.
+        // But for now, let's assume required non-'any' types cannot be null.
+        if ($value === null) {
+            // This depends on how schema/types define nullability.
+            // For now, if it's not 'any', and it's null, let's consider it not matching basic types like 'string', 'integer'.
+            // This might need adjustment based on broader type system design.
+            // For the specific test case (pAny: null), type 'any' handles it.
+            // If we had pString: null (required:true, type:string), this would make it fail here.
+            return false; // Example: 'string' does not accept null unless explicitly stated.
+        }
+
         return match ($type) {
             'string' => is_string($value),
             'number' => is_numeric($value),
             'integer' => is_int($value),
             'boolean' => is_bool($value),
             'array' => is_array($value),
             'object' => is_object($value),
-            // For 'any' or other custom types, we might need more sophisticated validation or assume true.
-            // 'any' type essentially means no type validation beyond presence if required.
-            'any' => true, // Explicitly allow 'any' type.
             default => true // Allow other unknown types by default, or could be stricter.
         };
     }

tests/Tool/Fixture/AllNullAnnotationsTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(title: null, readOnlyHint: null, destructiveHint: null, idempotentHint: null, openWorldHint: null)]
+class AllNullAnnotationsTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/DestructiveTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(destructiveHint: true)]
+class DestructiveTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/EmptyAnnotationsTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations]
+class EmptyAnnotationsTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/IdempotentTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(idempotentHint: true)]
+class IdempotentTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/LifecycleTestTool.php

@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+class LifecycleTestTool extends Tool
+{
+    public bool $initialized = false;
+    public bool $shutdown = false;
+    /** @var string[] */
+    public array $log = [];
+
+    public function __construct(?array $config = null)
+    {
+        $this->log[] = "Before parent constructor";
+        parent::__construct($config);
+        $this->log[] = "After parent constructor";
+    }
+
+    public function initialize(): void
+    {
+        parent::initialize(); // Good practice to call parent
+        $this->initialized = true;
+        $this->log[] = "initialize called";
+    }
+
+    public function shutdown(): void
+    {
+        parent::shutdown(); // Good practice to call parent
+        $this->shutdown = true;
+        $this->log[] = "shutdown called";
+    }
+
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("executed");
+    }
+
+    // Helper to get the log for assertions
+    /** @return string[] */
+    public function getLog(): array
+    {
+        return $this->log;
+    }
+
+    public function clearLog(): void
+    {
+        $this->log = [];
+    }
+}

tests/Tool/Fixture/OpenWorldTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(openWorldHint: true)]
+class OpenWorldTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/ReadOnlyTool.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(readOnlyHint: true)]
+class ReadOnlyTool extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/ToolWithAnnotations.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+#[ToolAnnotations(title: "Test Tool Annotations")]
+class ToolWithAnnotations extends Tool
+{
+    protected function doExecute(array $arguments): array|ContentItemInterface
+    {
+        return $this->text("test");
+    }
+}

tests/Tool/Fixture/ValidationTestTool.php

@@ -0,0 +1,25 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool\Fixture;
+
+use MCP\Server\Tool\Tool;
+use MCP\Server\Tool\Attribute\Parameter;
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+class ValidationTestTool extends Tool
+{
+    protected function doExecute(array $arguments): ContentItemInterface
+    {
+        $results = [];
+        // Iterate over expected keys to ensure consistent logging for test assertions
+        $expectedKeys = ['pInt', 'pObj', 'pAny', 'pOptInt', 'pOptObj', 'pOptAny'];
+        foreach ($expectedKeys as $key) {
+            if (array_key_exists($key, $arguments)) {
+                $results[] = "{$key} type: " . gettype($arguments[$key]);
+            }
+        }
+        return $this->text(implode(', ', $results));
+    }
+}

tests/Tool/ToolAnnotationsTest.php

@@ -0,0 +1,95 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MCP\Server\Tests\Tool;
+
+use PHPUnit\Framework\TestCase;
+use MCP\Server\Tool\Attribute\ToolAnnotations;
+use MCP\Server\Tests\Tool\Fixture\ToolWithAnnotations;
+use MCP\Server\Tests\Tool\Fixture\DestructiveTool;
+use MCP\Server\Tests\Tool\Fixture\IdempotentTool;
+use MCP\Server\Tests\Tool\Fixture\OpenWorldTool;
+use MCP\Server\Tests\Tool\Fixture\ReadOnlyTool;
+use MCP\Server\Tests\Tool\Fixture\EmptyAnnotationsTool;
+use MCP\Server\Tests\Tool\Fixture\AllNullAnnotationsTool;
+use MCP\Server\Tool\Tool; // Needed for the anonymous class test case
+use MCP\Server\Tool\Content\ContentItemInterface;
+
+// Needed for anonymous class
+
+
+class ToolAnnotationsTest extends TestCase
+{
+    public function testInitializeMetadataWithTitle(): void
+    {
+        $tool = new ToolWithAnnotations();
+        $annotations = $tool->getAnnotations();
+        $this->assertNotNull($annotations);
+        $this->assertArrayHasKey('title', $annotations);
+        $this->assertEquals('Test Tool Annotations', $annotations['title']);
+    }
+
+    public function testInitializeMetadataWithDestructiveHint(): void
+    {
+        $tool = new DestructiveTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNotNull($annotations);
+        $this->assertArrayHasKey('destructiveHint', $annotations);
+        $this->assertTrue($annotations['destructiveHint']);
+    }
+
+    public function testInitializeMetadataWithIdempotentHint(): void
+    {
+        $tool = new IdempotentTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNotNull($annotations);
+        $this->assertArrayHasKey('idempotentHint', $annotations);
+        $this->assertTrue($annotations['idempotentHint']);
+    }
+
+    public function testInitializeMetadataWithOpenWorldHint(): void
+    {
+        $tool = new OpenWorldTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNotNull($annotations);
+        $this->assertArrayHasKey('openWorldHint', $annotations);
+        $this->assertTrue($annotations['openWorldHint']);
+    }
+
+    public function testInitializeMetadataWithReadOnlyHint(): void
+    {
+        $tool = new ReadOnlyTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNotNull($annotations);
+        $this->assertArrayHasKey('readOnlyHint', $annotations);
+        $this->assertTrue($annotations['readOnlyHint']);
+    }
+
+    public function testInitializeMetadataWithNoAnnotations(): void
+    {
+        // Use an anonymous class that extends Tool without any ToolAnnotations attribute
+        $tool = new class extends Tool {
+            protected function doExecute(array $arguments): array|ContentItemInterface
+            {
+                return $this->text("test");
+            }
+        };
+        $annotations = $tool->getAnnotations();
+        $this->assertNull($annotations, "Annotations should be null when no ToolAnnotations attribute is present.");
+    }
+
+    public function testInitializeMetadataWithEmptyToolAnnotations(): void
+    {
+        $tool = new EmptyAnnotationsTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNull($annotations, "Annotations should be null when ToolAnnotations attribute is empty.");
+    }
+
+    public function testInitializeMetadataWithAllHintsNullInToolAnnotations(): void
+    {
+        $tool = new AllNullAnnotationsTool();
+        $annotations = $tool->getAnnotations();
+        $this->assertNull($annotations, "Annotations should be null when all hints in ToolAnnotations are null.");
+    }
+}