-
Notifications
You must be signed in to change notification settings - Fork 45
Fix/tool annotations mapping #91
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: trunk
Are you sure you want to change the base?
Changes from all commits
8d4a7ca
d9b7b73
1284837
ea1f25a
1a53442
e6c6f13
84036d3
a67eff3
1338f82
fd03768
4bd5906
e406505
410478f
5c62821
4279271
bdfd6f9
94c7456
00a6423
26dff6f
baee55a
06b44eb
18f4bc4
e90ac38
a324e79
6c1452c
ea8a12c
fd72336
f7a961a
578c6b7
b6bf7d7
e5a8284
f0d3e7f
beac29c
e23f669
17b57ca
d3e00ee
b3c3f4e
670e382
19a9f97
75bb8f4
7f0519f
13e50e8
5c639d3
44c49e6
5964afc
94fec28
8e10f8b
0fa7819
0b99a5b
0bb6289
b0df6c6
08f972d
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 |
|---|---|---|
|
|
@@ -66,9 +66,9 @@ class McpPrompt { | |
| /** | ||
| * The MCP server instance this prompt belongs to. | ||
| * | ||
| * @var \WP\MCP\Core\McpServer | ||
| * @var \WP\MCP\Core\McpServer|null | ||
| */ | ||
| private McpServer $mcp_server; | ||
| private ?McpServer $mcp_server = null; | ||
|
Member
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 confused why it's possible for the McpServer to be nullable. Is there a use case for this? Or should it be a required parameter in the constructor?
Member
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. Same question for Tool and Resource.
Contributor
Author
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. This allows flexible component registration, components can be created independently and then associated with a server during registration. I can modify this and make it more strict if you prefer.
Member
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 see. This actually raises a different question, then: Why are the prompts, tools, and resources coupled to a specific server instance, rather than something ingested by the server?
Contributor
Author
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. hhhh, yes, this creates a circular dependency:
Probably, we can check for uniqueness and validate it inside McpComponentRegistry (or something similar). I need to think about this.
Member
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. Yeah, I would think the dependency chain is Server > Registry > Tools/Resources/Prompts. Ideally, the dependency is uni-directional, so the components aren't aware of the registry or server, and the registry isn't aware of the server.
Contributor
Author
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. Should we address this on another issue/PR?
Member
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. Yeah, let's resolve this in a subsequent PR. 👍 |
||
|
|
||
| /** | ||
| * Constructor for McpPrompt. | ||
|
|
@@ -356,6 +356,13 @@ public static function from_array( array $data, McpServer $mcp_server ) { | |
| * @return self|\WP_Error Returns the validated prompt instance or WP_Error if validation fails. | ||
| */ | ||
| public function validate( string $context = '' ) { | ||
| if ( null === $this->mcp_server ) { | ||
| return new \WP_Error( | ||
| 'prompt_missing_mcp_server', | ||
| esc_html__( 'MCP server must be set before validating a prompt.', 'mcp-adapter' ) | ||
| ); | ||
| } | ||
|
|
||
| if ( ! $this->mcp_server->is_mcp_validation_enabled() ) { | ||
| return $this; | ||
| } | ||
|
|
@@ -401,6 +408,10 @@ public static function create_argument( string $name, ?string $description = nul | |
| * @return \WP\MCP\Core\McpServer | ||
| */ | ||
| public function get_mcp_server(): McpServer { | ||
| if ( null === $this->mcp_server ) { | ||
| throw new \RuntimeException( 'MCP server has not been set on this prompt instance.' ); | ||
| } | ||
|
|
||
| return $this->mcp_server; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.