jamesnetherton commented on code in PR #9458:
URL: https://github.com/apache/camel-quarkus/pull/9458#discussion_r4510462897
##########
integration-tests/mcp-server/src/test/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerTest.java:
##########
@@ -216,6 +252,19 @@ void testCallToolSuccess() {
.thenAssertResults();
}
+ /**
+ * An argument error is not sanitized the way a route failure is: the
client gets the validation message.
+ */
+ @Test
+ void testCallToolMissingRequiredArgument() {
Review Comment:
This test needs `say_hello` to be registered, but
`testStoppingToolRouteKeepsToolOfOtherServer` restarts `say-hello-route` in its
`finally` without waiting for the tool to be registered again (that happens
asynchronously, which is why `testToolsListReflectsRouteStopAndStart` uses
`await()`). Depending on method order, this test could get a "tool not found"
error instead of the expected `isError` response.
`testToolsListReflectsRouteStopAndStart` also has no `try`/`finally`, so if
it fails partway the route stays stopped and this test fails too, which hides
the real cause.
Waiting for `say_hello` to come back after each restart (or using a
dedicated route for the stop/start tests) would remove the order dependency.
##########
integration-tests/mcp-server/src/test/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerTest.java:
##########
@@ -205,6 +221,26 @@ void testQuarkusAnnotatedToolsCoexistWithCamelTools() {
.thenAssertResults();
}
+ /**
+ * MCP has a flat tool namespace: when an ai-tool route asks for a name an
annotated tool already owns, the
+ * annotated tool keeps the name and the route is refused. The route is
started with the application, which
+ * is the case that used to fail the startup.
+ */
+ @Test
+ void testCamelToolCollidingWithAnnotatedToolIsRefused() {
Review Comment:
`annotated-clash-route` is never stopped here, so nothing checks that
stopping a refused route leaves the `@Tool` with the same name in place. That
is only covered for the `ToolManager`-registered `user_tool`
(`testStoppingRefusedToolKeepsApplicationTool`). Could a stop/start of
`annotated-clash-route` be added, followed by a call to `clashing_tool`?
##########
extensions/mcp-server/runtime/src/main/doc/usage.adoc:
##########
@@ -147,9 +147,15 @@ public class CalculatorTools {
is exposed alongside the `ai-tool` routes. Annotated tools are registered at
build time; Camel tools are added and removed dynamically with the route
-lifecycle. Choose distinct tool names — MCP has a flat tool namespace: a
-Camel tool whose name is already taken by a tool of the application is
-refused with an ERROR log, while its route keeps running.
+lifecycle. `quarkus.camel.mcp-server.tags` only selects `ai-tool` routes:
+tools of the application are owned by quarkus-mcp-server and are exposed
+whatever the tags are, declaring `@Tool` being the opt-in.
Review Comment:
Tools registered programmatically through `ToolManager` (like the
`user_tool` fixture) also ignore the tags, and they don't use `@Tool`, so
"declaring `@Tool` being the opt-in" is not quite accurate. Maybe state just
the behaviour, e.g.:
```suggestion
lifecycle. `quarkus.camel.mcp-server.tags` only selects `ai-tool` routes.
Tools registered by the application, through `@Tool` or `ToolManager`, are
always exposed.
```
##########
integration-tests/mcp-server/src/test/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerTest.java:
##########
@@ -205,6 +221,26 @@ void testQuarkusAnnotatedToolsCoexistWithCamelTools() {
.thenAssertResults();
}
+ /**
+ * MCP has a flat tool namespace: when an ai-tool route asks for a name an
annotated tool already owns, the
+ * annotated tool keeps the name and the route is refused. The route is
started with the application, which
+ * is the case that used to fail the startup.
+ */
+ @Test
+ void testCamelToolCollidingWithAnnotatedToolIsRefused() {
+ assertThat(routeStatus("annotated-clash-route")).isEqualTo("Started");
+ client().when()
+ .toolsList(page ->
assertThat(toolNames(page)).containsOnlyOnce("clashing_tool"))
+ .toolsCall("clashing_tool", response -> {
+ assertThat(response.isError()).isNotEqualTo(Boolean.TRUE);
+ // the answer tells which of the two implementations
served the call
+ assertThat(textOf(response))
+
.isEqualTo(QuarkusDefinedTools.CLASHING_TOOL_ANSWER)
Review Comment:
The two constants are different strings, so
`.isNotEqualTo(McpServerRoutes.CLASHING_TOOL_ANSWER)` adds nothing after
`isEqualTo(...)`. Also, `isNotEqualTo(Boolean.TRUE)` on line 235 could simply
be `isFalse()`.
##########
integration-tests/mcp-server/src/test/java/org/apache/camel/quarkus/component/mcp/server/it/McpServerTest.java:
##########
@@ -205,6 +221,26 @@ void testQuarkusAnnotatedToolsCoexistWithCamelTools() {
.thenAssertResults();
}
+ /**
+ * MCP has a flat tool namespace: when an ai-tool route asks for a name an
annotated tool already owns, the
+ * annotated tool keeps the name and the route is refused. The route is
started with the application, which
+ * is the case that used to fail the startup.
+ */
+ @Test
+ void testCamelToolCollidingWithAnnotatedToolIsRefused() {
+ assertThat(routeStatus("annotated-clash-route")).isEqualTo("Started");
+ client().when()
+ .toolsList(page ->
assertThat(toolNames(page)).containsOnlyOnce("clashing_tool"))
Review Comment:
quarkus-mcp-server stores tools by name, so `tools/list` can never list the
same name twice and `containsOnlyOnce` cannot fail. The response text below is
the real check, so this could be `contains("clashing_tool")` or be dropped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
