-
Notifications
You must be signed in to change notification settings - Fork 624
Report an error if ToolArg.name is not set #32827 #32850
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
Report an error if ToolArg.name is not set #32827 #32850
Conversation
34d4eae
to
daf8d2a
Compare
d7a846d
to
a398d16
Compare
- Main source needs to depend on the no parameters source set - No parameters source set needs to define its own dependencies, separately from BND, since the bnd build has to depend on it. - Since we need to depend on jakarta APIs, we need the jakarta project to export individual configurations for the dependencies we need so we can reference them from gradle.
…en-liberty into feature-mcp-server-32827
ebc19e8
to
f9f39d7
Compare
@Tool(name = "illegalToolArgNameTool", title = "illegal ToolArgName Tool", description = "thows illegalArgumentExeption because of the ToolArgName") | ||
public String illegalToolArgNameTool(@ToolArg(name = Tool.ELEMENT_NAME, description = "input to echo") String input) { |
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.
What we're testing here is a missing name rather than an "illegal" name.
We should define the argument like this:
@ToolArg(description = "input to echo") String input
We should rename the test.
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.
Done, thanks
String expectedErrorHeader = "Illegal arguments found in MCP Tool:"; | ||
List<String> expectedErrorList = List.of("io.openliberty.mcp.internal.fat.noparamtool.NoParamTools.illegalToolArgNameTool"); | ||
ExpectedAppFailureValidator.findAndAssertExpectedErrorsInLogs("Illegal Tool Arg: ", expectedErrorHeader, expectedErrorList, server); |
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 message isn't descriptive enough to tell the user what the problem is and what they need to do.
The problem is that a name was not provided for the parameter.
The solution is to either add a name
to the @ToolArg
annotation, or to add the -parameters
compiler option to use the parameter name.
We don't have a name for the parameter, but we do know what position it's in so we can tell the user that.
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 the message now
E CWWKZ0002E: An exception occurred while starting the application ExpectedNoParamNameFailureTest. The exception message was: com.ibm.ws.container.service.state.StateChangeException: org.jboss.weld.exceptions.DeploymentException: Missing arguments found in MCP Tool: Tool: class io.openliberty.mcp.internal.fat.noparamtool.NoParamTools.missingToolArgNameTool Tool argument name was not provided for the parameter. Either add a name to the @ToolArg annotation, or to add the -parameters compiler option to use the parameter name
Could you squash the commits before merging please? |
…y, review comments review comment to make names consistent Co-authored-by: Andrew Rouse <[email protected]>
44e490b
to
7ad2d1e
Compare
release bug
label if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).Resolves Report an error if
ToolArg.name
is not set and the parameter name is not included in the class #32827