gnodet-bot commented on code in PR #27512:
URL: https://github.com/apache/camel/pull/27512#discussion_r4209611170
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogService.java:
##########
@@ -79,8 +79,21 @@ public CamelCatalog getDefaultCatalog() {
public CamelCatalog loadCatalog(String runtime, String camelVersion,
String platformBom) throws Exception {
RuntimeType runtimeType = resolveRuntime(runtime);
- boolean hasVersion = camelVersion != null && !camelVersion.isBlank();
+ // "main", "latest", "current" or "default" (what an assistant writes
when it means the version in use) is
+ // the default catalog, not a version to download
+ boolean hasVersion = camelVersion != null && !camelVersion.isBlank()
&& !camelVersion.isEmpty()
Review Comment:
🔵 **Low — Redundant `isEmpty()` check:** `isBlank()` returns `true` for both
empty and blank strings, so `!camelVersion.isBlank()` being `true` already
guarantees `!camelVersion.isEmpty()` is `true`. The `isEmpty()` check is never
reached when `isBlank()` is false.
```suggestion
boolean hasVersion = camelVersion != null && !camelVersion.isBlank()
```
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/MigrationTools.java:
##########
@@ -211,11 +212,12 @@ public CompatibilityResult camel_migration_compatibility(
+ "BEFORE running the OpenRewrite recipes. If the
project does not compile, fix the build "
+ "errors first. OpenRewrite requires a compilable
project to parse and transform the code.")
public MigrationRecipesResult camel_migration_recipes(
- @ToolArg(description = ToolArgDocs.RUNTIME_REQUIRED) String
runtime,
+ @ToolArg(description = ToolArgDocs.RUNTIME_REQUIRED, required =
false) String runtime,
Review Comment:
⚠️ **High — Schema contradiction:** The annotation says `required = false`,
but the description uses `ToolArgDocs.RUNTIME_REQUIRED` (the constant name
literally says REQUIRED) and the method body immediately throws if `runtime` is
null or blank. An LLM client reading the JSON schema will see the parameter as
optional and may omit it — resulting in a `ToolCallException`. Either set
`required = true` here (matching the null-guard), or update the guard to handle
the missing case gracefully.
```suggestion
@ToolArg(description = ToolArgDocs.RUNTIME_REQUIRED) String
runtime,
```
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogService.java:
##########
@@ -121,11 +134,38 @@ public CamelCatalog loadCatalog(String runtime, String
camelVersion, String plat
return cached;
}
- CamelCatalog loaded = doLoadCatalog(runtimeType, camelVersion,
platformBomGav);
+ CamelCatalog loaded;
+ try {
+ loaded = doLoadCatalog(runtimeType, camelVersion, platformBomGav);
+ } catch (Exception e) {
+ if (runtimeType == RuntimeType.main && hasVersion && !hasBom) {
+ // a version that cannot be downloaded (not released, no
network): answer from the default catalog
+ // rather than fail the tool call
+ return defaultCatalog;
Review Comment:
🟡 **Medium — Silent fallback with no logging:** When a version cannot be
downloaded, `defaultCatalog` is returned silently. A user passing e.g.
`camelVersion="4.99.0"` will get back the default catalog version with no
warning or indication that the requested version wasn't found. Consider adding
at least a `LOG.warn("Could not load catalog for version {}, falling back to
default: {}", camelVersion, e.getMessage())` before the return.
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/RuntimeTools.java:
##########
@@ -394,18 +400,24 @@ For deep analysis, use a full heap dump (jmap -dump:live)
with tools like Eclips
Entries with lowConfidence=true have unreliable growth
percentages due to low sample counts or \
sample counts that diverge significantly between runs —
recommend a longer recording duration.""")
public JsonObject camel_runtime_memory_leak(
- @ToolArg(description = NAME_OR_PID_DESC) String nameOrPid,
+ @ToolArg(description = NAME_OR_PID_DESC, required = false) String
nameOrPid,
@ToolArg(description = "Command: start, stop, status, or query")
String command,
- @ToolArg(description = "Recording duration in seconds (only for
start command, default 60, use 0 for manual stop)") String duration,
- @ToolArg(description = "Recording mode: dual (default, two
recordings at Xs and 2Xs with trend comparison) or single (one recording)")
String mode,
- @ToolArg(description = "Include allocation stack traces in results
(default false, set true for detailed analysis)") String stacktrace,
- @ToolArg(description = "Minimum total size in bytes to include a
sample (e.g. 1024 for 1KB). Filters out small allocations to reduce noise.
Default 1024 (1KB) in dual mode") String minSize) {
+ @ToolArg(description = "Recording duration in seconds (only for
start command, default 60, use 0 for manual stop)",
+ required = false) String duration,
+ @ToolArg(description = "Recording mode: dual (default, two
recordings at Xs and 2Xs with trend comparison) or single (one recording)",
+ required = false) String mode,
+ @ToolArg(description = "Include allocation stack traces in results
(default false, set true for detailed analysis)",
+ required = false) String stacktrace,
+ @ToolArg(description = "Minimum total size in bytes to include a
sample (e.g. 1024 for 1KB). Filters out small allocations to reduce noise.
Default 1024 (1KB) in dual mode",
+ required = false) String minSize) {
if (command == null || command.isBlank()) {
throw new ToolCallException("command is required (start, stop,
status, or query)", null);
}
RuntimeService.ProcessInfo p =
runtimeService.findSingleProcess(nameOrPid);
- if ("start".equals(command) && "dual".equalsIgnoreCase(mode)) {
+ // dual is the documented default: an omitted or blank mode records
twice too
+ boolean dual = mode == null || mode.isBlank() ||
"dual".equalsIgnoreCase(mode);
Review Comment:
🔵 **Low — Behavioral change not covered by a test:** When `mode` is `null`
or blank (i.e., the argument is omitted), this now triggers dual recording.
Previously omitting `mode` was a no-op. The PR description documents this as
intentional, but there's no new test asserting that `mode=null` → dual path.
Worth adding a test case.
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/CatalogService.java:
##########
@@ -121,11 +134,38 @@ public CamelCatalog loadCatalog(String runtime, String
camelVersion, String plat
return cached;
}
- CamelCatalog loaded = doLoadCatalog(runtimeType, camelVersion,
platformBomGav);
+ CamelCatalog loaded;
+ try {
+ loaded = doLoadCatalog(runtimeType, camelVersion, platformBomGav);
+ } catch (Exception e) {
+ if (runtimeType == RuntimeType.main && hasVersion && !hasBom) {
+ // a version that cannot be downloaded (not released, no
network): answer from the default catalog
+ // rather than fail the tool call
+ return defaultCatalog;
+ }
+ throw e;
+ }
cache.putIfAbsent(key, loaded);
return cache.get(key);
}
+ /** Whether the value is an org.apache.camel groupId:artifactId:version
GAV. */
+ static boolean isCamelGav(String gav) {
+ String[] parts = gav.trim().split(":");
+ return parts.length == 3 && "org.apache.camel".equals(parts[0].trim())
&& !parts[1].isBlank() && !parts[2].isBlank();
+ }
+
+ /** Whether the version is the default catalog's, with or without a
-SNAPSHOT qualifier. */
+ boolean sameAsDefault(String camelVersion) {
+ String mine = defaultCatalog.getCatalogVersion();
+ if (mine == null) {
+ return false;
+ }
+ String v = camelVersion.trim();
+ return v.equals(mine) || v.equals(mine.replace("-SNAPSHOT", ""))
+ || mine.equals(v.replace("-SNAPSHOT", ""));
Review Comment:
🔵 **Low — Third arm intent is non-obvious:**
`mine.equals(v.replace("-SNAPSHOT", ""))` handles the case where the user asks
for `4.22.0-SNAPSHOT` and the default is `4.22.0` (the inverse of the second
arm). A brief comment would help readers understand why both directions are
needed:
```suggestion
// second arm: user asks for 4.22.0, default is 4.22.0-SNAPSHOT →
same
// third arm: user asks for 4.22.0-SNAPSHOT, default is 4.22.0 → same
return v.equals(mine) || v.equals(mine.replace("-SNAPSHOT", ""))
|| mine.equals(v.replace("-SNAPSHOT", ""));
```
--
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]
