davsclaus commented on code in PR #27840:
URL: https://github.com/apache/camel/pull/27840#discussion_r4275140171
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/AuthoringTools.java:
##########
@@ -367,18 +371,46 @@ public JsonObject camel_save_project_summary(
}
/**
- * As {@link #call(String, Map)} for a deterministic tool: from the third
identical call of the connection the
- * answer is a short note that it was already answered (CAMEL-25075).
+ * As {@link #call(String, Map, CatalogService)} for a deterministic tool:
from the third identical call of the
+ * connection the answer is a short note that it was already answered
(CAMEL-25075). Only successful answers count
+ * against the repeat budget; a call that throws does not (CAMEL-25640).
*/
JsonObject callDeterministic(McpConnection connection, String tool,
Map<String, String> args) {
JsonObject repeat = repeatedCalls != null ?
repeatedCalls.repeatOf(connection, tool, args) : null;
- return repeat != null ? repeat : call(tool, args);
+ if (repeat != null) {
+ return repeat;
+ }
+ JsonObject result = call(tool, args, catalogService);
+ if (repeatedCalls != null) {
+ repeatedCalls.recordSuccessfulAnswer(connection, tool, args);
+ }
+ return result;
}
/** Runs the registry tool of the same name and hands its JSON back; a
tool error becomes an MCP tool error. */
static JsonObject call(String tool, Map<String, String> args) {
+ return call(tool, args, null);
+ }
+
+ /**
+ * Runs the registry tool of the same name and hands its JSON back; a tool
error becomes an MCP tool error. When
+ * {@code catalogService} is provided, the {@link ToolContext} reuses the
shared catalog cache instead of creating a
+ * new one per call (CAMEL-25640).
+ */
+ static JsonObject call(String tool, Map<String, String> args,
CatalogService catalogService) {
try {
- return RuntimeTools.toJsonObject(ToolRegistry.execute(tool, new
ToolContext(), args));
+ ToolContext ctx = new ToolContext();
+ if (catalogService != null) {
+ String camelVersion = args != null ? args.get("camelVersion")
: null;
+ String runtime = args != null ? args.get("runtime") : null;
Review Comment:
None of the four tools that reach this method (`camel_catalog_doc`, `_find`,
`_sample`, `camel_validate_source`) has a `runtime` argument, so this line does
nothing today. If a tool such as `camel_dependency_for_class`, whose `runtime`
means something else, is ever routed through here, it would quietly get a
Spring Boot or Quarkus catalog. I would pass `null` here.
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/AuthoringTools.java:
##########
@@ -367,18 +371,46 @@ public JsonObject camel_save_project_summary(
}
/**
- * As {@link #call(String, Map)} for a deterministic tool: from the third
identical call of the connection the
- * answer is a short note that it was already answered (CAMEL-25075).
+ * As {@link #call(String, Map, CatalogService)} for a deterministic tool:
from the third identical call of the
+ * connection the answer is a short note that it was already answered
(CAMEL-25075). Only successful answers count
+ * against the repeat budget; a call that throws does not (CAMEL-25640).
*/
JsonObject callDeterministic(McpConnection connection, String tool,
Map<String, String> args) {
JsonObject repeat = repeatedCalls != null ?
repeatedCalls.repeatOf(connection, tool, args) : null;
- return repeat != null ? repeat : call(tool, args);
+ if (repeat != null) {
+ return repeat;
+ }
+ JsonObject result = call(tool, args, catalogService);
+ if (repeatedCalls != null) {
+ repeatedCalls.recordSuccessfulAnswer(connection, tool, args);
+ }
+ return result;
}
/** Runs the registry tool of the same name and hands its JSON back; a
tool error becomes an MCP tool error. */
static JsonObject call(String tool, Map<String, String> args) {
+ return call(tool, args, null);
+ }
+
+ /**
+ * Runs the registry tool of the same name and hands its JSON back; a tool
error becomes an MCP tool error. When
+ * {@code catalogService} is provided, the {@link ToolContext} reuses the
shared catalog cache instead of creating a
+ * new one per call (CAMEL-25640).
+ */
+ static JsonObject call(String tool, Map<String, String> args,
CatalogService catalogService) {
try {
- return RuntimeTools.toJsonObject(ToolRegistry.execute(tool, new
ToolContext(), args));
+ ToolContext ctx = new ToolContext();
+ if (catalogService != null) {
+ String camelVersion = args != null ? args.get("camelVersion")
: null;
+ String runtime = args != null ? args.get("runtime") : null;
+ try {
+ CamelCatalog catalog = catalogService.loadCatalog(runtime,
camelVersion, null);
+ ctx.setCatalog(catalog);
Review Comment:
When `camelVersion` is given, this catalog never gets used. Every
deterministic registry tool begins with `applyVersion(ctx, args)`, which calls
`ctx.setCamelVersion(version)`. On this fresh context the version is null, so
the two differ and `setCamelVersion` sets `catalog = null`.
`ToolContext.loadCatalog()` then downloads through `CatalogLoader` again, with
no cache and without `camel.catalog.repos`. A version that cannot be downloaded
is now tried twice per call (here, and again in the context). Setting the
version first keeps the catalog, because `setCamelVersion` with the same value
is a no-op:
```java
if (camelVersion != null && !camelVersion.isBlank()) {
ctx.setCamelVersion(camelVersion.trim());
}
ctx.setCatalog(catalog);
```
(`CatalogService` falls back to the default catalog when a download fails,
so that becomes the answer instead of an error. Please decide that on purpose.)
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogService.java:
##########
@@ -208,7 +208,15 @@ private CamelCatalog doLoadCatalog(
if (runtimeType == RuntimeType.springBoot) {
return CatalogLoader.loadSpringBootCatalog(repos,
camelVersion, true);
} else if (runtimeType == RuntimeType.quarkus) {
- return CatalogLoader.loadQuarkusCatalog(platformBom,
downloader()::resolveArtifact);
+ // Resolve the Quarkus platform BOM that matches the requested
Camel version.
+ // platformBom is null here (only camelVersion was given), so
we must look it up.
+ MavenGav resolvedBom = QuarkusHelper.findQuarkusPlatformBom(
+ camelVersion,
+ downloader()::resolveArtifact,
+ true,
+ RuntimeType.QUARKUS_EXTENSION_REGISTRY_BASE_URL,
+ false).quarkusCamelBom();
Review Comment:
Non-blocking: `findQuarkusPlatformBom` returns null when the registry cannot
be read or no platform matches, and then `.quarkusCamelBom()` throws an NPE.
The tool answers with `(java.lang.NullPointerException)`. A
`ToolCallException("No Quarkus platform found for Camel " + camelVersion,
null)` would give the model something it can act on. The no-version path at
`:111-117` has the same issue.
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogService.java:
##########
@@ -218,6 +226,8 @@ private CamelCatalog doLoadCatalog(
if (runtimeType == RuntimeType.springBoot) {
return CatalogLoader.loadSpringBootCatalog(repos, null, true);
} else if (runtimeType == RuntimeType.quarkus) {
+ // platformBom should have been resolved in loadCatalog() before
doLoadCatalog() is called; passing null
+ // here would silently produce an unusable catalog, so we guard
with an assertion to surface the bug early.
return CatalogLoader.loadQuarkusCatalog(platformBom,
downloader()::resolveArtifact);
Review Comment:
There is no assertion. As far as I can see, this branch cannot be reached:
every Quarkus call to `doLoadCatalog` comes with either a BOM or a version.
Drop the comment, or make the guard real:
```suggestion
// loadCatalog() always resolves the platform BOM for quarkus
before calling doLoadCatalog()
throw new IllegalStateException("No Quarkus platform BOM
resolved");
```
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiMcpServer.java:
##########
@@ -532,10 +532,15 @@ private JsonObject handleToolsCall(JsonObject request) {
String text;
boolean isError = false;
+ ToolDescriptor tool = toolName != null ?
ToolRegistry.findTool(toolName) : null;
try {
- JsonObject repeat = toolName != null
- ? repeatedCalls.repeatOf(ToolRegistry.findTool(toolName),
args) : null;
- text = repeat != null ? repeat.toJson() :
toolRegistry.execute(toolName, args);
+ JsonObject repeat = repeatedCalls.repeatOf(tool, args);
+ if (repeat != null) {
+ text = repeat.toJson();
+ } else {
+ text = toolRegistry.execute(toolName, args);
+ repeatedCalls.recordSuccessfulAnswer(tool, args);
Review Comment:
For the shared `camel_*` tools, `TuiToolRegistry.executeShared` catches
`ToolExecutionException` and returns `"Error: " + message` as normal text, so
`execute()` does not throw. A failed `camel_catalog_doc` (a version that cannot
be downloaded) or a `camel_validate_source` without `file` is therefore
recorded as a successful answer here. On the Camel TUI path, failed calls still
use up the budget. Either `executeShared` should report the error to this
method (throw, or return a flag so the result also gets `isError`), or this
should skip the record for an error result.
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/KameletTools.java:
##########
@@ -53,15 +73,16 @@ public KameletListResult camel_catalog_kamelets(
@ToolArg(description = "Apache Camel Kamelets version. If not
specified, uses the default version.",
required = false) String kameletsVersion) {
- int maxResults = limit != null ? limit : 50;
+ int maxResults = limit != null ? Math.max(1, limit) : 50;
Review Comment:
Non-blocking: with this, `limit=0` returns one Kamelet. The JIRA suggests
the default for `<= 0`, as `ExampleTools` does:
```suggestion
int maxResults = limit != null && limit > 0 ? limit : 50;
```
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogTools.java:
##########
@@ -53,7 +53,7 @@ public DocListResult camel_catalog_docs(
@ToolArg(description = "Maximum number of results to return
(default: 50)", required = false) Integer limit,
@ToolArg(description = ToolArgDocs.CAMEL_VERSION, required =
false) String camelVersion) {
- int maxResults = limit != null ? limit : 50;
+ int maxResults = limit != null ? Math.max(1, limit) : 50;
Review Comment:
Same as in `KameletTools`. This change is also not mentioned in the PR
description.
```suggestion
int maxResults = limit != null && limit > 0 ? limit : 50;
```
##########
dsl/camel-jbang/camel-jbang-mcp/src/test/java/org/apache/camel/dsl/jbang/core/commands/mcp/KameletToolsTest.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.dsl.jbang.core.commands.mcp;
+
+import java.util.List;
+
+import io.quarkiverse.mcp.server.ToolCallException;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * CAMEL-25640: KameletTools bugs — no caching and limit-before-sort.
+ */
+class KameletToolsTest {
+
+ private final KameletTools tools = new KameletTools();
+
+ /** CAMEL-25640: repeated calls must return identical, sorted results (no
HashMap iteration order). */
+ @Test
+ void repeatedCallsReturnTheSameSortedList() {
+ KameletTools.KameletListResult first =
tools.camel_catalog_kamelets(null, null, null, null);
+ KameletTools.KameletListResult second =
tools.camel_catalog_kamelets(null, null, null, null);
+
+ assertThat(first.kamelets()).isEqualTo(second.kamelets());
+ // kamelets are sorted by name
+ List<String> names =
first.kamelets().stream().map(KameletTools.KameletInfo::name).toList();
+ List<String> sorted =
names.stream().sorted(String.CASE_INSENSITIVE_ORDER).toList();
+ assertThat(names).isEqualTo(sorted);
+ }
+
+ /** CAMEL-25640: limit is applied after sorting, so the first N kamelets
alphabetically are returned. */
+ @Test
+ void limitIsAppliedAfterSorting() {
+ KameletTools.KameletListResult full =
tools.camel_catalog_kamelets(null, null, null, null);
+ assertThat(full.kamelets()).hasSizeGreaterThan(5);
+
+ KameletTools.KameletListResult limited =
tools.camel_catalog_kamelets(null, null, 3, null);
+ assertThat(limited.count()).isEqualTo(3);
+ // total must reflect the full set size, not the limited subset
+ assertThat(limited.total()).isEqualTo(full.total());
+ // the 3 kamelets returned must be the first 3 alphabetically of the
full set
+ assertThat(limited.kamelets())
+ .isEqualTo(full.kamelets().subList(0, 3));
+ }
+
+ /** CAMEL-25640: total reflects all matching kamelets, not just the
returned page. */
+ @Test
+ void totalReflectsAllMatchingKamelets() {
+ KameletTools.KameletListResult all =
tools.camel_catalog_kamelets(null, null, null, null);
+ KameletTools.KameletListResult paged =
tools.camel_catalog_kamelets(null, null, 1, null);
+
+ assertThat(paged.count()).isEqualTo(1);
+ assertThat(paged.total()).isEqualTo(all.total());
+ }
+
+ @Test
+ void filterByTypeReturnsOnlyMatchingType() {
+ KameletTools.KameletListResult sources =
tools.camel_catalog_kamelets(null, "source", null, null);
+ assertThat(sources.kamelets()).isNotEmpty();
+ assertThat(sources.kamelets()).allMatch(k ->
"source".equalsIgnoreCase(k.type()));
+ }
+
+ @Test
+ void filterByNameOrDescriptionSubstring() {
+ KameletTools.KameletListResult aws =
tools.camel_catalog_kamelets("aws-s3", null, null, null);
+ assertThat(aws.kamelets()).isNotEmpty();
+ assertThat(aws.kamelets()).allMatch(
+ k -> k.name() != null &&
k.name().toLowerCase().contains("aws-s3")
+ || k.description() != null &&
k.description().toLowerCase().contains("aws-s3"));
+ }
+
+ @Test
+ void kameletDocReturnsDetails() {
+ KameletTools.KameletDetailResult detail =
tools.camel_catalog_kamelet_doc("log-sink", null);
+ assertThat(detail.name()).isEqualTo("log-sink");
+ assertThat(detail.type()).isEqualTo("sink");
+ assertThat(detail.options()).isNotEmpty();
+ }
+
+ @Test
+ void kameletDocForUnknownNameThrows() {
+ assertThatThrownBy(() ->
tools.camel_catalog_kamelet_doc("no-such-kamelet-xyz", null))
+ .isInstanceOf(ToolCallException.class)
+ .hasMessageContaining("no-such-kamelet-xyz");
+ }
+
+ @Test
+ void kameletDocForBlankNameThrows() {
+ assertThatThrownBy(() -> tools.camel_catalog_kamelet_doc("", null))
+ .isInstanceOf(ToolCallException.class)
+ .hasMessageContaining("required");
+ }
+
+ /** CAMEL-25640: second call uses the cache, not a fresh loadKamelets(). */
+ @Test
+ void secondCallIsServedFromCache() {
+ // two calls with the same version — should not throw and should be
equal
+ KameletTools.KameletListResult r1 = tools.camel_catalog_kamelets(null,
null, null, null);
+ KameletTools.KameletListResult r2 = tools.camel_catalog_kamelets(null,
null, null, null);
+ assertThat(r1.kamelets()).isEqualTo(r2.kamelets());
+ assertThat(r1.kameletsVersion()).isEqualTo(r2.kameletsVersion());
Review Comment:
Non-blocking: this passes without the cache too, since it only compares two
results. The whole class also downloads `camel-kamelets-catalog` from Maven
Central (the module has no dependency on it) and is not disabled in CI like the
other download tests here. A package-private loader hook that counts the loads
would test the cache and keep the test offline.
##########
dsl/camel-jbang/camel-jbang-mcp/src/test/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogServiceTest.java:
##########
@@ -176,4 +176,20 @@ void
downloadedCatalogViaPlatformBomVersionDiffersFromBuiltin() {
assertThat(version).isEqualTo(requestedVersion);
assertThat(version).isNotEqualTo(BUILTIN_VERSION);
}
+
+ /**
+ * CAMEL-25640: quarkus runtime with only camelVersion (no explicit
platformBom) used to pass null to
+ * loadQuarkusCatalog, which silently returned an unusable catalog. The
fix resolves the BOM from the registry.
+ * Disabled in CI: requires network access to the Quarkus extension
registry.
+ */
+ @Test
+ @DisabledIfSystemProperty(named = "ci.env.name", matches = ".*",
+ disabledReason = "Runs only local — requires
Quarkus extension registry access")
+ void quarkusRuntimeWithCamelVersionResolvesTheBomFromRegistry() {
+ // Verify that passing camelVersion with quarkus runtime does not
silently pass null to loadQuarkusCatalog.
+ // A successful return (even if it is the built-in version) means no
NPE was thrown inside doLoadCatalog.
+ CatalogService service = createService(null);
+ String result = version(service, "quarkus", BUILTIN_VERSION, null);
+ assertThat(result).isNotEmpty();
Review Comment:
`BUILTIN_VERSION` is the default catalog's version, so `sameAsDefault()`
sets `hasVersion = false`, and `loadCatalog` takes the older no-version Quarkus
path, not the new branch in `doLoadCatalog`. A released version that is not the
built-in one (e.g. `4.18.0`), with an assertion that the returned catalog is
for that major.minor, would cover the fix.
--
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]
