gnodet-bot commented on code in PR #27840:
URL: https://github.com/apache/camel/pull/27840#discussion_r4275823010
##########
dsl/camel-jbang/camel-jbang-mcp/src/main/java/org/apache/camel/dsl/jbang/core/commands/mcp/AuthoringTools.java:
##########
@@ -367,18 +371,50 @@ 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;
+ try {
+ // Set the version first: setCamelVersion() on a fresh
context nulls out any previously set catalog,
+ // so the catalog must be set after (or the version set
before, so it is a no-op when they match).
+ if (camelVersion != null && !camelVersion.isBlank()) {
+ ctx.setCamelVersion(camelVersion.trim());
+ }
+ CamelCatalog catalog = catalogService.loadCatalog(null,
camelVersion, null);
+ ctx.setCatalog(catalog);
+ } catch (Exception e) {
+ // catalog load failed: fall through to ToolContext's own
lazy-load, which handles the error
Review Comment:
⚠️ **Medium:** This catch block silently discards the first failure from
`CatalogService.loadCatalog()` with no log output. The fallback to
`ToolContext`'s own lazy-load is correct and will surface its own error if it
also fails, but the first attempt's exception is completely invisible. On a
slow or flaky network this doubles download latency with zero diagnostic trace.
The comment documents the intent well, but a `DEBUG`/`TRACE` log is a
zero-cost improvement for production observability:
```suggestion
} catch (Exception e) {
// catalog load failed: fall through to ToolContext's
own lazy-load, which handles the error
LOG.debug("CatalogService.loadCatalog() failed ({}),
falling back to ToolContext lazy-load", e.getMessage());
```
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiMcpServer.java:
##########
@@ -532,10 +532,19 @@ 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);
+ // executeShared catches ToolExecutionException and returns
"Error: ..." as normal text; only
+ // record the answer when the call actually succeeded so
failed calls do not use the retry budget
+ if (!text.startsWith("Error:")) {
Review Comment:
💡 **Low:** `startsWith("Error:")` couples success detection to a string
convention that is spread across `executeShared()` (which catches
`ToolExecutionException` and returns `"Error: " + msg`) and 30+ hardcoded
`return "Error: ..."` strings in TUI tool handlers. The convention is locally
consistent (all producers are in the same file), so the practical risk today is
low, but any format change silently breaks the retry-budget accounting.
A typed result would make this more robust — e.g. have
`toolRegistry.execute()` return a wrapper with a success/failure flag, or
propagate a checked exception instead of stringifying errors. At minimum, a
constant for the error prefix would prevent silent drift:
```suggestion
if (!text.startsWith("Error:")) {
```
→ consider extracting `"Error:"` to a shared constant (e.g.
`TuiToolRegistry.ERROR_PREFIX`) so that the detection site and the producers
stay in sync without reading each other's source code.
--
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]
