gnodet-bot commented on code in PR #27931: URL: https://github.com/apache/camel/pull/27931#discussion_r4290546661
########## dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/CliRunner.java: ########## @@ -0,0 +1,187 @@ +/* + * 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; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import org.apache.camel.dsl.jbang.core.common.CliExitCode; +import org.apache.camel.dsl.jbang.core.common.Printer; +import org.apache.camel.dsl.jbang.core.common.StringPrinter; +import org.apache.camel.util.json.JsonObject; +import picocli.CommandLine; + +/** + * Runs a Camel CLI command in this JVM, as camel ask and the TUI do, and gives what a script running camel gets: the + * exit code, and stdout and stderr apart. The commands share the printer of {@link CamelJBangMain} and the picocli + * command tree, so one command runs at a time: the camel shell takes the same {@link #LOCK}. + */ +public final class CliRunner { + + /** + * The lock of the commands that run in this JVM. + */ + public static final Object LOCK = CamelJBangMain.class; + + private CliRunner() { + } + + /** + * What a command did. + * + * @param exitCode the exit code, as camel exits with it ({@link CliExitCode#TIMEOUT} when it did not finish in + * time) + * @param stdout the output of the command + * @param stderr the errors, warnings and tips + * @param elapsedMs how long the command ran + * @param timedOut whether the command did not finish in time, and was interrupted + */ + public record CliResult(int exitCode, String stdout, String stderr, long elapsedMs, boolean timedOut) { + + public boolean isSuccess() { + return exitCode == CliExitCode.OK; + } + + /** + * The result as JSON, for an AI agent: exitCode, stdout, stderr, elapsedMs and timedOut, where stdout and + * stderr are cut at the given number of characters. + */ + public JsonObject toJson(int maxChars) { + JsonObject answer = new JsonObject(); + answer.put("exitCode", exitCode); + answer.put("stdout", cut(stdout, maxChars)); + answer.put("stderr", cut(stderr, maxChars)); + answer.put("elapsedMs", elapsedMs); + answer.put("timedOut", timedOut); + return answer; + } + + private static String cut(String text, int maxChars) { + return text.length() > maxChars ? text.substring(0, maxChars) + "\n... (truncated)" : text; + } + } + + /** + * Runs the command, and waits until it has finished. + * + * @param args the command line without camel, such as <tt>get route --json</tt> + */ + public static CliResult run(List<String> args) { + return run(args, 0); + } + + /** + * Runs the command, and interrupts it when it does not finish in time. + * + * @param args the command line without camel, such as <tt>get route --json</tt> + * @param timeoutMillis how long the command may run, or 0 to wait until it has finished + */ + public static CliResult run(List<String> args, long timeoutMillis) { + CommandLine commandLine = CamelJBangMain.getCommandLine(); + if (commandLine == null) { + throw new IllegalStateException("The Camel CLI is not initialized"); + } + Output output = new Output(); + long start = System.nanoTime(); + if (timeoutMillis <= 0) { + int code = execute(commandLine, args, output); + return output.result(code, elapsed(start), false); + } + + FutureTask<Integer> task = new FutureTask<>(() -> execute(commandLine, args, output)); + Thread thread = new Thread(task, "camel-cli-runner"); + thread.setDaemon(true); + thread.start(); + try { + int code = task.get(timeoutMillis, TimeUnit.MILLISECONDS); + return output.result(code, elapsed(start), false); + } catch (TimeoutException e) { + task.cancel(true); Review Comment: ⚠️ **Potential lock starvation on timeout** `task.cancel(true)` sends an interrupt, but `execute()` holds `synchronized (LOCK)` for the entire duration of `commandLine.execute(...)`. If the picocli command is blocked on non-interruptible I/O or a native call that ignores interrupts, the daemon thread keeps holding `LOCK` until the I/O unblocks — which could be never. Every subsequent CLI command (e.g. from the TUI) calling `execute()` will block waiting on `LOCK`, effectively hanging the shell. Consider adding a `thread.join(someGraceMillis)` after the cancel and logging a warning if the thread is still alive, so operators can detect a stuck lock. ########## dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/TuiToolRegistry.java: ########## @@ -1868,6 +1874,8 @@ String callRunExample(Map<String, Object> args) throws Exception { if (outcome.log() != null) { result.put("log", outcome.log()); } + // a tool error (MCP isError), which still has the log that tells why + throw new ToolExecutionException(Jsoner.serialize(result)); Review Comment: ⚠️ **`callRunExample` throws JSON inside `ToolExecutionException`, which gets `"Error: "` prepended** `throw new ToolExecutionException(Jsoner.serialize(result))` stores a serialized JSON object as the exception message. `TuiMcpServer.handleToolsCall` catches this as `Exception e` at line 552–553 and sets `text = "Error: " + e.getMessage()`, producing a string like `Error: {"status":"failed",...}`. That is not valid JSON, so any MCP client that tries to parse the tool error content as JSON (e.g. to extract `log`) will fail. The intent seems to be that `ToolExecutionException` should be caught separately and its message used directly as `text` (with `isError = true`) — the same way `UnknownToolException` is caught at line 548. Add a `catch (ToolExecutionException e)` before the generic `catch (Exception e)` block in `handleToolsCall` that sets `text = e.getMessage()` without the prefix. -- 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]
