-
Notifications
You must be signed in to change notification settings - Fork 81
[Server] Add tool name validation for SEP-986 #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the official PHP MCP SDK. | ||
| * | ||
| * A collaboration between Symfony and the PHP Foundation. | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Mcp\Capability\Tool; | ||
|
|
||
| final class NameValidator | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m not sure we need to extract name validation into a separate class here. Tool names must follow a hard rule according the spec and not a user-configurable validation. So having dedicated Also, if we introduce a standalone validator for tool names, then for consistency we’d have to do the same for resource name validation ( Since Wdyt? |
||
| { | ||
| public function isValid(string $name): bool | ||
| { | ||
| return 1 === preg_match('/^[a-zA-Z0-9._\/-]{1,64}$/', $name); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the official PHP MCP SDK. | ||
| * | ||
| * A collaboration between Symfony and the PHP Foundation. | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Mcp\Tests\Unit\Capability\Tool; | ||
|
|
||
| use Mcp\Capability\Tool\NameValidator; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class NameValidatorTest extends TestCase | ||
| { | ||
| #[DataProvider('provideValidNames')] | ||
| public function testValidNames(string $name): void | ||
| { | ||
| $this->assertTrue((new NameValidator())->isValid($name)); | ||
| } | ||
|
|
||
| public static function provideValidNames(): array | ||
| { | ||
| return [ | ||
| ['my_tool'], | ||
| ['MyTool123'], | ||
| ['my.tool'], | ||
| ['my-tool'], | ||
| ['my/tool'], | ||
| ['my_tool-01.02'], | ||
| ['my_long_toolname_that_is_exactly_sixty_four_characters_long_1234'], | ||
| ]; | ||
| } | ||
|
|
||
| #[DataProvider('provideInvalidNames')] | ||
| public function testInvalidNames(string $name): void | ||
| { | ||
| $this->assertFalse((new NameValidator())->isValid($name)); | ||
| } | ||
|
|
||
| public static function provideInvalidNames(): array | ||
| { | ||
| return [ | ||
| [''], | ||
| ['my tool'], | ||
| ['my@tool'], | ||
| ['my!tool'], | ||
| ['my_tool#1'], | ||
| ['this_tool_name_is_way_too_long_because_it_exceeds_the_sixty_four_character_limit_set_by_the_validator'], | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is addressing array shapes for DataProvider methods in tests - i don't think there is any value in defining them since it is only PHPUnit consuming those arrays not user land code.