-
Notifications
You must be signed in to change notification settings - Fork 14
Add validation for prompt name uniqueness across the service. #833
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
Conversation
|
||
/// Definition of the prompt template | ||
value: PromptTemplateDefinition | ||
} | ||
|
||
@pattern("^[a-zA-Z0-9]+(?:_[a-zA-Z0-9]+)*$") |
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.
Currently keeping it as characters, allowed with _
separators and numbers.
8736e35
to
46202c2
Compare
var promptTemplateDefinition = entry.getValue(); | ||
var templateString = promptTemplateDefinition.getTemplate(); | ||
|
||
// Generate the new prompt name with service prefix | ||
var promptName = serviceName + "__" + originalPromptName; |
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.
Some clients (e.g. q) will automatically prefix tool names with the servers that provide them. Will our manual implementation clash?
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.
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.
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.
Hmm, that is a good point. For prompts, I don't see a limit in the MCP schema or host like tools. I can play around and verify it. However, we can choose to enforce a limit ourselves on the prompts and service name. Alternatively expose an id
field in the trait that let's the customer define what its called?
We can always reduce this in future if we want. It should be a good start for now?
For now IMO this disambiguation works pretty well across services. For instance, DynamoDB__safe_delete_table and Glue__safe_delete_table.
What do you think?
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.
I would avoid till we have a use case to add.
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.
Ack. so as part of the PR, the uniqueness validator and naming @pattern
is still good to push? If yes, I will update this PR.
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.
Discussed offline, and we also wanted a check in terms of server side uniqueness. I have added that check instead of adding the ServiceName___PromptName
convention for now. If we see issues with the server in future, we should add the unique naming logic at that time.
smithy-ai-traits/src/main/java/software/amazon/smithy/ai/PromptUniquenessValidator.java
Outdated
Show resolved
Hide resolved
0a4dec5
to
81b0d9a
Compare
|
||
if (prompts.containsKey(normalizedName)) { | ||
var existingPrompt = prompts.get(normalizedName); | ||
throw new RuntimeException(String.format( |
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.
Is there a better exception I can throw here that will probably alert user that the server isn't loading due to prompt names?
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.
The message should be surfaced to the user.
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.
Currently the only way would be to surface this at client during runtime, when the MCP Client starts it up.
mcp/mcp-server/src/main/java/software/amazon/smithy/java/mcp/server/PromptLoader.java
Show resolved
Hide resolved
…ll as in a single smithy model at build time.
81b0d9a
to
d550570
Compare
Add validation for prompt name uniqueness across services
Changes
The prompts are checked for uniqueness of name case-insensitively following same rules as defined for ShapeIds in the semantics of a Smithy Model.
Examples of conflict
Invalid - would cause validation errors
Validation Error:
Prompt name 'Get_Employee_Info' conflicts with existing prompt 'get_employee_info' in service EmployeeService (case-insensitive comparison)
After (Valid - unique names):
Testing
Validation changes
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.