AI Assistant hardening #8757

config-doc/pom.xml

@@ -41,6 +41,22 @@
             <version>${jandex.version}</version>
         </dependency>
 
+        <!-- Configuration classes are instantiated to read their defaults. Some refer to hop-core
+             types, such as an enum that implements IEnumHasCodeAndDescription, so hop-core itself
+             has to be on the classpath; what it depends on does not. -->
+        <dependency>
+            <groupId>org.apache.hop</groupId>
+            <artifactId>hop-core</artifactId>
+            <version>${project.version}</version>
+            <scope>provided</scope>
+            <exclusions>
+                <exclusion>
+                    <groupId>*</groupId>
+                    <artifactId>*</artifactId>
+                </exclusion>
+            </exclusions>
+        </dependency>
+
         <!-- The hop-* dependencies below are not used in code. They are the modules that declare a
              @ConfigPlugin, and they are here so the reactor builds this one after all of them: the
              generator reads the whole tree's build output, so running it earlier would document a

config-doc/src/main/java/org/apache/hop/config/doc/ConfigDocGenerator.java

@@ -476,11 +476,14 @@ static String body(Plugin p) {
       if (isLongDefault(o.dflt())) {
         // An AsciiDoc cell, so the default can be a block under the description where there is
         // room for it. The Default column is a narrow one and a long value wraps to a word a line.
-        sb.append("a|")
-            .append(text(description))
-            .append("\n\n.Default\n[listing]\n----\n")
-            .append(o.dflt().strip())
-            .append("\n----\n");
+        // Not a listing block: that never wraps, and its longest line sets the width of the whole
+        // table. One line of inline code per line of the value keeps the line breaks and wraps.
+        sb.append("a|").append(text(description)).append("\n\n.Default\n[%hardbreaks]\n");
+        for (String line : o.dflt().strip().split("\\R")) {
+          if (!line.isBlank()) {
+            sb.append(literal(line.strip())).append("\n");
+          }
+        }
         sb.append("|_(with the description)_\n\n");
       } else {
         sb.append("|").append(text(description)).append("\n");

core/src/main/java/org/apache/hop/ai/advisor/AiAdvisorOpenRequest.java

@@ -54,6 +54,13 @@ public class AiAdvisorOpenRequest {
   private String focusNodeName;
   private Object artifact;
   private Supplier<String> logSupplier;
+
+  /**
+   * Identifies the latest run of the pipeline or workflow, for example its log channel id, or
+   * returns null before the first run. A new value means the log belongs to a new run.
+   */
+  private Supplier<String> runIdSupplier;
+
   private boolean reuseExisting = true;
 
   /**

core/src/main/java/org/apache/hop/ai/advisor/AiAdvisorResponse.java

@@ -33,6 +33,12 @@ public class AiAdvisorResponse {
   /** True when the raw text contained a {@code hop_proposals} fence, even if it did not parse. */
   private boolean proposalBlockPresent;
 
+  /**
+   * Why a {@code hop_proposals} block could not be read, for example invalid JSON, or null when it
+   * was read or there was none.
+   */
+  private String proposalParseError;
+
   /** Prompt tokens reported by the provider, if any. */
   private Integer inputTokenCount;
 

core/src/main/java/org/apache/hop/ai/advisor/AiProposalParser.java

@@ -18,7 +18,6 @@
 package org.apache.hop.ai.advisor;
 
 import com.fasterxml.jackson.databind.JsonNode;
-import com.fasterxml.jackson.databind.ObjectMapper;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -56,37 +55,63 @@ public static AiAdvisorResponse parse(String rawResponse) {
     List<AiProposal> proposals = new ArrayList<>();
     boolean blockPresent = false;
     Matcher matcher = PROPOSAL_BLOCK.matcher(rawResponse);
+    String error = null;
     while (matcher.find()) {
       blockPresent = true;
       advice = advice.replace(matcher.group(0), "").trim();
-      proposals.addAll(parseProposalJson(matcher.group(1)));
+      try {
+        proposals.addAll(parseProposalJson(matcher.group(1)));
+      } catch (IllegalArgumentException e) {
+        error = e.getMessage();
+      }
     }
     response.setProposalBlockPresent(blockPresent);
+    response.setProposalParseError(error);
     response.setMarkdownAdvice(advice.trim());
     response.setProposals(proposals);
     return response;
   }
 
+  /**
+   * @throws IllegalArgumentException with a short reason when the block is not a proposals object
+   */
   private static List<AiProposal> parseProposalJson(String jsonText) {
     List<AiProposal> proposals = new ArrayList<>();
     if (Utils.isEmpty(jsonText)) {
-      return proposals;
+      throw new IllegalArgumentException("The hop_proposals block is empty.");
     }
+    JsonNode root;
     try {
-      ObjectMapper mapper = HopJson.newMapper();
-      JsonNode root = mapper.readTree(jsonText.trim());
-      JsonNode array = root.path("proposals");
-      if (!array.isArray()) {
-        return proposals;
-      }
-      for (JsonNode node : array) {
-        AiProposal proposal = toProposal(node);
-        if (proposal != null) {
-          proposals.add(proposal);
+      root = HopJson.newMapper().readTree(jsonText.trim());
+    } catch (Exception e) {
+      String reason = e.getMessage() == null ? e.getClass().getSimpleName() : e.getMessage();
+      int newline = reason.indexOf('\n');
+      throw new IllegalArgumentException(
+          "The hop_proposals block is not valid JSON: "
+              + (newline > 0 ? reason.substring(0, newline) : reason));
+    }
+    JsonNode array = root == null ? null : root.path("proposals");
+    if (array == null || !array.isArray()) {
+      throw new IllegalArgumentException(
+          "The hop_proposals block has no \"proposals\" array at the top level.");
+    }
+    for (JsonNode node : array) {
+      AiProposal proposal = toProposal(node);
+      if (proposal != null) {
+        // Small models copy a list of allowed types into one proposal ("ADD_TRANSFORM|ADD_HOP").
+        // That can never be applied; reading it as unreadable gets it corrected.
+        String type = proposal.getType();
+        if (type != null && (type.contains("|") || type.contains(","))) {
+          throw new IllegalArgumentException(
+              "Proposal '"
+                  + (Utils.isEmpty(proposal.getDescription()) ? type : proposal.getDescription())
+                  + "' has several types ("
+                  + type
+                  + "). Give each proposal exactly one type: adding a transform and its hop takes"
+                  + " two proposals.");
         }
+        proposals.add(proposal);
       }
-    } catch (Exception ignored) {
-      // Malformed blocks are dropped; advice text is still shown.
     }
     return proposals;
   }
@@ -95,10 +120,8 @@ private static AiProposal toProposal(JsonNode node) {
     if (node == null || node.isNull()) {
       return null;
     }
+    // An item without a type is kept: the validator blocks it and the user sees why.
     String typeValue = node.path("type").asText("");
-    if (Utils.isEmpty(typeValue)) {
-      return null;
-    }
     AiProposal proposal = new AiProposal();
     String id = node.path("id").asText("");
     proposal.setId(Utils.isEmpty(id) ? null : id);

core/src/main/java/org/apache/hop/ai/advisor/AiProposalValidation.java

@@ -32,4 +32,15 @@ public class AiProposalValidation {
   private boolean blocked;
   private String reason;
   private String warning;
+
+  /**
+   * Not selected in the review by default: the user has to choose it. Used for changes that remove
+   * or replace existing work, such as deletes, replacements and overwritten metadata.
+   */
+  private boolean optIn;
+
+  /** The constructor from before {@link #optIn} existed, kept for advisors in other plugins. */
+  public AiProposalValidation(String proposalId, boolean blocked, String reason, String warning) {
+    this(proposalId, blocked, reason, warning, false);
+  }
 }

core/src/main/java/org/apache/hop/ai/provider/AiProviderObjectFactory.java

@@ -21,30 +21,66 @@
 import org.apache.hop.core.exception.HopMissingPluginsException;
 import org.apache.hop.core.plugins.IPlugin;
 import org.apache.hop.core.plugins.PluginRegistry;
+import org.apache.hop.core.util.Utils;
+import org.apache.hop.i18n.BaseMessages;
 import org.apache.hop.metadata.api.IHopMetadataObjectFactory;
 
 /** Instantiates {@link IAiProvider} plugins by id when deserializing AI provider metadata. */
 public class AiProviderObjectFactory implements IHopMetadataObjectFactory {
 
+  private static final Class<?> PKG = AiProviderObjectFactory.class;
+
+  /**
+   * The id written by earlier versions that saved a provider without its plugin id. Such a file
+   * loads as a provider without a type, so the user can pick one and save it again.
+   */
+  static final String MISSING_ID = "null";
+
   @Override
   public Object createObject(String id, Object parentObject)
       throws HopException, HopMissingPluginsException {
+    if (Utils.isEmpty(id) || MISSING_ID.equals(id)) {
+      return new MissingTypeAiProvider();
+    }
     PluginRegistry registry = PluginRegistry.getInstance();
     IPlugin plugin = registry.findPluginWithId(AiProviderPluginType.class, id);
     if (plugin == null) {
       HopMissingPluginsException missing =
-          new HopMissingPluginsException("AI provider plugin not found: " + id);
+          new HopMissingPluginsException(
+              BaseMessages.getString(PKG, "AiProviderObjectFactory.PluginNotFound", id));
       missing.addMissingPluginDetails(AiProviderPluginType.class, id);
       throw missing;
     }
-    return registry.loadClass(plugin);
+    Object object = registry.loadClass(plugin);
+    if (object instanceof IAiProvider provider) {
+      provider.setPluginId(plugin.getIds()[0]);
+      provider.setPluginName(plugin.getName());
+    }
+    return object;
   }
 
   @Override
   public String getObjectId(Object object) throws HopException {
     if (!(object instanceof IAiProvider provider)) {
-      throw new HopException("Object is not an IAiProvider but " + object.getClass().getName());
+      throw new HopException(
+          BaseMessages.getString(
+              PKG, "AiProviderObjectFactory.NotAProvider", object.getClass().getName()));
     }
-    return provider.getPluginId();
+    String pluginId = provider.getPluginId();
+    if (Utils.isEmpty(pluginId)) {
+      IPlugin plugin = PluginRegistry.getInstance().getPlugin(AiProviderPluginType.class, object);
+      if (plugin != null) {
+        pluginId = plugin.getIds()[0];
+        provider.setPluginId(pluginId);
+        provider.setPluginName(plugin.getName());
+      }
+    }
+    if (Utils.isEmpty(pluginId)) {
+      throw new HopException(BaseMessages.getString(PKG, "AiProviderObjectFactory.NoType"));
+    }
+    return pluginId;
   }
+
+  /** Stands in for a provider that was saved without its plugin id. It has no type. */
+  static final class MissingTypeAiProvider extends BaseAiProvider {}
 }

core/src/main/resources/org/apache/hop/ai/provider/messages/messages_en_US.properties

@@ -0,0 +1,20 @@
+#
+# 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.
+#
+
+AiProviderObjectFactory.PluginNotFound=AI provider type ''{0}'' is not installed. Install the plugin that provides it, or open the provider in the Metadata perspective and select another type.
+AiProviderObjectFactory.NotAProvider=Object is not an IAiProvider but {0}
+AiProviderObjectFactory.NoType=The AI provider has no provider type. Select a provider type before saving.

core/src/test/java/org/apache/hop/ai/advisor/AiProposalParserTest.java

@@ -19,6 +19,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.junit.jupiter.api.Test;
@@ -76,6 +77,46 @@ void malformedBlockIsDroppedButAdviceRemains() {
     assertTrue(response.isProposalBlockPresent());
     assertTrue(response.getProposals().isEmpty());
     assertTrue(response.getMarkdownAdvice().contains("Keep this advice."));
+    assertTrue(response.getProposalParseError().contains("not valid JSON"));
+  }
+
+  @Test
+  void blockWithoutProposalsArrayReportsWhy() {
+    String raw =
+        """
+        ```hop_proposals
+        [{"type":"ADD_TRANSFORM"}]
+        ```
+        """;
+    AiAdvisorResponse response = AiProposalParser.parse(raw);
+    assertTrue(response.getProposals().isEmpty());
+    assertTrue(response.getProposalParseError().contains("\"proposals\" array"));
+  }
+
+  @Test
+  void combinedTypesAreReportedSoTheModelCanCorrectThem() {
+    String raw =
+        """
+        ```hop_proposals
+        {"proposals":[{"description":"Add Dummy","type":"ADD_TRANSFORM|ADD_PIPELINE_HOP"}]}
+        ```
+        """;
+    AiAdvisorResponse response = AiProposalParser.parse(raw);
+    assertTrue(response.getProposalParseError().contains("several types"));
+  }
+
+  @Test
+  void itemWithoutTypeIsKeptSoTheValidatorCanBlockIt() {
+    String raw =
+        """
+        ```hop_proposals
+        {"proposals":[{"description":"Add a filter","parameters":{"name":"Filter"}}]}
+        ```
+        """;
+    AiAdvisorResponse response = AiProposalParser.parse(raw);
+    assertEquals(1, response.getProposals().size());
+    assertEquals("", response.getProposals().get(0).getType());
+    assertNull(response.getProposalParseError());
   }
 
   @Test

core/src/test/java/org/apache/hop/ai/provider/AiProviderPluginTypeTest.java

@@ -20,6 +20,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -79,6 +80,21 @@ void objectFactoryReturnsPluginId() throws Exception {
     assertEquals("fake-openai", new AiProviderObjectFactory().getObjectId(provider));
   }
 
+  @Test
+  void objectFactoryLoadsProviderSavedWithoutType() throws Exception {
+    Object object = new AiProviderObjectFactory().createObject("null", null);
+    assertTrue(object instanceof IAiProvider);
+    assertNull(((IAiProvider) object).getPluginId());
+  }
+
+  @Test
+  void objectFactoryRefusesToSaveProviderWithoutType() throws Exception {
+    PluginRegistry.addPluginType(AiProviderPluginType.getInstance());
+    AiProviderObjectFactory factory = new AiProviderObjectFactory();
+    Object object = factory.createObject("null", null);
+    assertThrows(HopException.class, () -> factory.getObjectId(object));
+  }
+
   @Test
   void baseProviderCloneCopiesIdentity() {
     FakeProvider original = new FakeProvider();