mattcasters commented on code in PR #9186: URL: https://github.com/apache/hop/pull/9186#discussion_r4435532160
########## plugins/tech/copilot/src/main/java/org/apache/hop/ai/copilot/GitHubCopilotProvider.java: ########## @@ -0,0 +1,313 @@ +/* + * 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.hop.ai.copilot; + +import com.github.copilot.CopilotClient; +import com.github.copilot.generated.AssistantMessageEvent; +import com.github.copilot.rpc.CopilotClientMode; +import com.github.copilot.rpc.CopilotClientOptions; +import com.github.copilot.rpc.MessageOptions; +import com.github.copilot.rpc.ModelInfo; +import com.github.copilot.rpc.PermissionHandler; +import com.github.copilot.rpc.PermissionRequestResult; +import com.github.copilot.rpc.SessionConfig; +import java.nio.file.Path; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import lombok.Getter; +import lombok.Setter; +import org.apache.hop.ai.AiAuthKind; +import org.apache.hop.ai.provider.AiChatMessage; +import org.apache.hop.ai.provider.AiChatRequest; +import org.apache.hop.ai.provider.AiChatResponse; +import org.apache.hop.ai.provider.AiProviderPlugin; +import org.apache.hop.ai.provider.BaseAiProvider; +import org.apache.hop.ai.provider.IAiChatProvider; +import org.apache.hop.ai.provider.IAiProvider; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.gui.plugin.GuiElementType; +import org.apache.hop.core.gui.plugin.GuiPlugin; +import org.apache.hop.core.gui.plugin.GuiWidgetElement; +import org.apache.hop.core.util.Utils; +import org.apache.hop.core.variables.IVariables; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.metadata.api.HopMetadataProperty; + +/** GitHub Copilot CLI-backed chat provider for Hop's AI Assistant. */ +@Getter +@Setter +@GuiPlugin(classLoaderGroup = "hop-ai-copilot") +@AiProviderPlugin( + id = "github-copilot", + name = "GitHub Copilot", + description = "GitHub Copilot CLI", + documentationUrl = "/metadata-types/ai-provider.html", + classLoaderGroup = "hop-ai-copilot") +public class GitHubCopilotProvider extends BaseAiProvider implements IAiChatProvider { + + private static final Class<?> PKG = GitHubCopilotProvider.class; + private static final long DEFAULT_TIMEOUT_SECONDS = 60; + private static final String COPILOT_CLI_PATH = "COPILOT_CLI_PATH"; + static final String DEFAULT_CLI_COMMAND = "copilot"; + + /** + * The session gets no tools, so nothing should ask for permission. Should the CLI ask anyway, the + * answer is no: an answer from the assistant may not run commands or touch files. + */ + static final PermissionHandler DENY_ALL = + (request, invocation) -> + CompletableFuture.completedFuture( + PermissionRequestResult.reject("Hop does not allow GitHub Copilot to use tools.")); + + @HopMetadataProperty + @GuiWidgetElement( + id = "0100-copilot-cli-path", + order = "0100", + type = GuiElementType.TEXT, + parentId = IAiProvider.GUI_PLUGIN_ELEMENT_PARENT_ID, + label = "i18n::GitHubCopilotProvider.CliPath.Label", + toolTip = "i18n::GitHubCopilotProvider.CliPath.Tooltip") + private String cliPath = ""; + + public GitHubCopilotProvider() { + setAuthKind(AiAuthKind.OAUTH); + setRequiresApiKey(false); + setHopModelType("COPILOT"); + } + + @Override + public boolean supportsBaseUrl() { + return false; + } + + @Override + public boolean supportsTemperature() { + return false; + } + + @Override + public boolean supportsContextSize() { + return false; + } + + @Override + public boolean supportsMaxOutputTokens() { + return false; + } + + @Override + public boolean supportsThinking() { + return false; + } + + @Override + public boolean supportsStructuredAnswers() { + return false; + } + + @Override + public AiChatResponse chat(AiChatRequest request, IVariables variables) throws HopException { + long started = System.nanoTime(); + long timeoutSeconds = timeoutSeconds(request); + try (CopilotClient client = newClient(variables)) { + await(client.start(), started, timeoutSeconds, "starting the Copilot CLI"); + SessionConfig config = sessionConfig(request.modelName()); + var session = + await( + client.createSession(config), started, timeoutSeconds, "creating a Copilot session"); + try (session) { + AssistantMessageEvent response = + await( + session.sendAndWait(new MessageOptions().setPrompt(prompt(request))), Review Comment: **[bug]** `CopilotSession.sendAndWait(MessageOptions)` always waits 60 seconds (`sendAndWait(options, 60000)` in SDK 1.0.28). The provider timeout (the editor's Timeout field, carried on `AiChatRequest.timeout` and enforced by `await`) is ignored for the model call. A value above 60 seconds still dies at 60 seconds. The SDK completes that future with `TimeoutException`, so `Future.get` throws `ExecutionException`, not `TimeoutException`. The outer `catch (TimeoutException)` never runs and the user sees `GitHubCopilotProvider.Failed` instead of the timeout message. `timeoutMs <= 0` on the two-arg overload means no SDK timeout. **Suggestion:** Call `sendAndWait(message, remainingMillis)` with the time still left in `timeoutSeconds` (at least 1 ms). Keep the outer `await` as a backstop for CLI startup and session creation. ########## plugins/tech/copilot/pom.xml: ########## @@ -0,0 +1,51 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.apache.hop</groupId> + <artifactId>hop-plugins-tech</artifactId> + <version>2.21.0-SNAPSHOT</version> + </parent> + + <artifactId>hop-tech-copilot</artifactId> + <packaging>jar</packaging> + <name>Hop Plugins Technology GitHub Copilot</name> + + <properties> + <copilot-sdk.version>1.0.28</copilot-sdk.version> + <hop.pluginlib.exclude1>jackson-databind</hop.pluginlib.exclude1> + <hop.pluginlib.exclude2>jackson-annotations</hop.pluginlib.exclude2> + <hop.pluginlib.exclude3>jna</hop.pluginlib.exclude3> + </properties> + + <dependencies> + <dependency> + <groupId>com.github</groupId> + <artifactId>copilot-sdk-java</artifactId> Review Comment: **[bug]** `copilot-sdk-java` 1.0.28 initializes its JSON-RPC mapper with `new JavaTimeModule()` (`JsonRpcClient.createObjectMapper`). That class lives in `jackson-datatype-jsr310`, which is not on Hop's core classpath, and `plugins/pom.xml` always drops artifact `jackson-datatype-jsr310` from `stage-plugin-lib`. The `hop.pluginlib.exclude*` properties only drop databind, annotations, and JNA; they do not put jsr310 back. The first `CopilotClient` use therefore fails with `NoClassDefFoundError: com/fasterxml/jackson/datatype/jsr310/JavaTimeModule` (an `Error`, so `chat` / `listModelNames` do not turn it into a `HopException`). Unit tests never construct a client, so they stay green. Databind and annotations are parent-first (`com.fasterxml.jackson.databind.` / `.annotation.`), so Hop's Jackson 2.21.7 is what the SDK actually runs on even if 2.22 jars were bundled. **Suggestion:** Ship `jackson-datatype-jsr310` at Hop's Jackson version inside this plugin and override the `stage-plugin-lib` exclude list so that artifact is copied. Adding the dependency alone is not enough while the parent exclude stays in place. ########## plugins/tech/copilot/src/main/java/org/apache/hop/ai/copilot/GitHubCopilotProvider.java: ########## @@ -0,0 +1,313 @@ +/* + * 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.hop.ai.copilot; + +import com.github.copilot.CopilotClient; +import com.github.copilot.generated.AssistantMessageEvent; +import com.github.copilot.rpc.CopilotClientMode; +import com.github.copilot.rpc.CopilotClientOptions; +import com.github.copilot.rpc.MessageOptions; +import com.github.copilot.rpc.ModelInfo; +import com.github.copilot.rpc.PermissionHandler; +import com.github.copilot.rpc.PermissionRequestResult; +import com.github.copilot.rpc.SessionConfig; +import java.nio.file.Path; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import lombok.Getter; +import lombok.Setter; +import org.apache.hop.ai.AiAuthKind; +import org.apache.hop.ai.provider.AiChatMessage; +import org.apache.hop.ai.provider.AiChatRequest; +import org.apache.hop.ai.provider.AiChatResponse; +import org.apache.hop.ai.provider.AiProviderPlugin; +import org.apache.hop.ai.provider.BaseAiProvider; +import org.apache.hop.ai.provider.IAiChatProvider; +import org.apache.hop.ai.provider.IAiProvider; +import org.apache.hop.core.exception.HopException; +import org.apache.hop.core.gui.plugin.GuiElementType; +import org.apache.hop.core.gui.plugin.GuiPlugin; +import org.apache.hop.core.gui.plugin.GuiWidgetElement; +import org.apache.hop.core.util.Utils; +import org.apache.hop.core.variables.IVariables; +import org.apache.hop.i18n.BaseMessages; +import org.apache.hop.metadata.api.HopMetadataProperty; + +/** GitHub Copilot CLI-backed chat provider for Hop's AI Assistant. */ +@Getter +@Setter +@GuiPlugin(classLoaderGroup = "hop-ai-copilot") +@AiProviderPlugin( + id = "github-copilot", + name = "GitHub Copilot", + description = "GitHub Copilot CLI", + documentationUrl = "/metadata-types/ai-provider.html", + classLoaderGroup = "hop-ai-copilot") +public class GitHubCopilotProvider extends BaseAiProvider implements IAiChatProvider { + + private static final Class<?> PKG = GitHubCopilotProvider.class; + private static final long DEFAULT_TIMEOUT_SECONDS = 60; + private static final String COPILOT_CLI_PATH = "COPILOT_CLI_PATH"; + static final String DEFAULT_CLI_COMMAND = "copilot"; + + /** + * The session gets no tools, so nothing should ask for permission. Should the CLI ask anyway, the + * answer is no: an answer from the assistant may not run commands or touch files. + */ + static final PermissionHandler DENY_ALL = + (request, invocation) -> + CompletableFuture.completedFuture( + PermissionRequestResult.reject("Hop does not allow GitHub Copilot to use tools.")); + + @HopMetadataProperty + @GuiWidgetElement( + id = "0100-copilot-cli-path", + order = "0100", + type = GuiElementType.TEXT, + parentId = IAiProvider.GUI_PLUGIN_ELEMENT_PARENT_ID, + label = "i18n::GitHubCopilotProvider.CliPath.Label", + toolTip = "i18n::GitHubCopilotProvider.CliPath.Tooltip") + private String cliPath = ""; + + public GitHubCopilotProvider() { + setAuthKind(AiAuthKind.OAUTH); + setRequiresApiKey(false); + setHopModelType("COPILOT"); + } + + @Override + public boolean supportsBaseUrl() { + return false; + } + + @Override + public boolean supportsTemperature() { + return false; + } + + @Override + public boolean supportsContextSize() { + return false; + } + + @Override + public boolean supportsMaxOutputTokens() { + return false; + } + + @Override + public boolean supportsThinking() { + return false; + } + + @Override + public boolean supportsStructuredAnswers() { + return false; + } + + @Override + public AiChatResponse chat(AiChatRequest request, IVariables variables) throws HopException { + long started = System.nanoTime(); + long timeoutSeconds = timeoutSeconds(request); + try (CopilotClient client = newClient(variables)) { + await(client.start(), started, timeoutSeconds, "starting the Copilot CLI"); + SessionConfig config = sessionConfig(request.modelName()); + var session = + await( + client.createSession(config), started, timeoutSeconds, "creating a Copilot session"); + try (session) { + AssistantMessageEvent response = + await( + session.sendAndWait(new MessageOptions().setPrompt(prompt(request))), + started, + timeoutSeconds, + "waiting for a Copilot response"); + return new AiChatResponse( + Utils.isEmpty(response.getData().content()) ? "" : response.getData().content(), Review Comment: **[bug]** `sendAndWait` is documented to complete with null when the session goes idle without an assistant message (`lastAssistantMessage` stays null and the future completes normally). This line calls `response.getData()` with no null check, so that outcome is an uncaught `NullPointerException` rather than an empty answer or a `HopException`. The same line NPEs if the event is non-null but `getData()` is null. `AiChatResponse` already turns a null text into `""`. **Suggestion:** If `response` or `response.getData()` is null, throw a `HopException` that says the CLI returned no assistant message (or return an empty `AiChatResponse`). Only then read `content()`. -- 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]
