gnodet-bot commented on code in PR #26567: URL: https://github.com/apache/camel/pull/26567#discussion_r4040896761
########## dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AnswerChecks.java: ########## @@ -0,0 +1,221 @@ +/* + * 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.ai; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; + +import org.apache.camel.catalog.CamelCatalog; +import org.apache.camel.catalog.DefaultCamelCatalog; +import org.apache.camel.catalog.LanguageValidationResult; +import org.apache.camel.tooling.model.LanguageModel; + +/** + * The checks of what an AI model says, as opposed to what it writes: a file goes through the validator before it is + * written, an answer shown in a chat does not, and it is what the user copies. The simple expressions of an answer are + * checked against the catalog, the way {@code camel_validate_source} checks a route: the {@code ${...}} placeholders of + * the text and of any code block that is not YAML, and the {@code simple:} values and log messages of the YAML blocks. + * <p> + * A placeholder is only checked when it starts with a simple function or value (body, header, exchangeProperty, random, + * date, ...), so a Maven {@code ${camel-version}} or a shell variable in the same answer is left alone. What the + * catalog cannot judge is skipped as in {@link SimpleChecks}: a function of a language that is not on the classpath, a + * property placeholder used as a logical operand. + */ +public final class AnswerChecks { + + /** A simple expression of an answer that the catalog rejects: what was written, and why. */ + public record Problem(String expression, String error) { + + /** {@code ${header.user ?: 'Guest'}: Unexpected token ?: at location 13}. */ + public String message() { + return expression + ": " + error; + } + } + + private static volatile CamelCatalog defaultCatalog; + private static volatile Set<String> roots; Review Comment: ⚠️ **`roots` field caches globally but is keyed to the wrong scope** Two problems with the current design: 1. **Missing DCL**: `defaultCatalog` uses proper double-checked locking (`synchronized (AnswerChecks.class)` + `volatile`), but `roots` uses bare `volatile` with a check-then-act pattern — two threads can both observe `null`, both compute the set, and one overwrites the other. In a UI panel that runs an agent on a background thread, this is a realistic race. 2. **Cache ignores the `catalog` argument**: `roots(CamelCatalog catalog)` takes a catalog parameter but caches the result in a static field keyed to nothing. If `checkSimple(text, customCatalog)` is called before `checkSimple(text)` (the default-catalog path), `roots` is populated from `customCatalog`'s functions and then returned as-is for every subsequent call regardless of which catalog is passed. In practice both are `DefaultCamelCatalog` so functions are identical — but the contract is broken: the catalog parameter to `checkSimple(String, CamelCatalog)` is silently ignored for the `isSimple` filter. Fix: apply the same DCL pattern as `defaultCatalog()`, and either (a) cache `roots` globally from the default catalog (since Simple functions are well-known and catalog-independent in practice), or (b) remove the `catalog` param from `roots()` and make the cache unconditionally use the default catalog for the roots set: ```suggestion private static volatile Set<String> roots; ``` And in the `roots()` method, apply DCL: ```java private static Set<String> roots(CamelCatalog catalog) { Set<String> answer = roots; if (answer == null) { synchronized (AnswerChecks.class) { answer = roots; if (answer == null) { answer = new HashSet<>(); LanguageModel simple = catalog.languageModel("simple"); if (simple != null && simple.getFunctions() != null) { for (LanguageModel.LanguageFunctionModel fn : simple.getFunctions()) { // ... same as now ... } } roots = answer; } } } return answer; } ``` ########## dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AnswerChecks.java: ########## @@ -0,0 +1,221 @@ +/* + * 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.ai; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; + +import org.apache.camel.catalog.CamelCatalog; +import org.apache.camel.catalog.DefaultCamelCatalog; +import org.apache.camel.catalog.LanguageValidationResult; +import org.apache.camel.tooling.model.LanguageModel; + +/** + * The checks of what an AI model says, as opposed to what it writes: a file goes through the validator before it is + * written, an answer shown in a chat does not, and it is what the user copies. The simple expressions of an answer are + * checked against the catalog, the way {@code camel_validate_source} checks a route: the {@code ${...}} placeholders of + * the text and of any code block that is not YAML, and the {@code simple:} values and log messages of the YAML blocks. + * <p> + * A placeholder is only checked when it starts with a simple function or value (body, header, exchangeProperty, random, + * date, ...), so a Maven {@code ${camel-version}} or a shell variable in the same answer is left alone. What the + * catalog cannot judge is skipped as in {@link SimpleChecks}: a function of a language that is not on the classpath, a + * property placeholder used as a logical operand. + */ +public final class AnswerChecks { + + /** A simple expression of an answer that the catalog rejects: what was written, and why. */ + public record Problem(String expression, String error) { + + /** {@code ${header.user ?: 'Guest'}: Unexpected token ?: at location 13}. */ + public String message() { + return expression + ": " + error; + } + } + + private static volatile CamelCatalog defaultCatalog; + private static volatile Set<String> roots; + + private AnswerChecks() { + } + + /** The problems of the simple expressions of an answer, checked against the default catalog. */ + public static List<Problem> checkSimple(String markdown) { + return checkSimple(markdown, defaultCatalog()); + } + + /** The problems of the simple expressions of an answer, checked against the given catalog. */ + public static List<Problem> checkSimple(String markdown, CamelCatalog catalog) { + if (markdown == null || markdown.isBlank() || catalog == null) { + return List.of(); + } + Map<String, Problem> problems = new LinkedHashMap<>(); + StringBuilder text = new StringBuilder(); + StringBuilder block = null; + boolean yaml = false; + for (String line : markdown.split("\n", -1)) { + String trimmed = line.trim(); + if (trimmed.startsWith("```")) { + if (block == null) { + String language = trimmed.substring(3).trim().toLowerCase(Locale.ROOT); + yaml = language.equals("yaml") || language.equals("yml"); + block = new StringBuilder(); + } else { + if (yaml) { + for (String error : SimpleChecks.validateYamlSimple(block.toString(), catalog)) { + problems.putIfAbsent(error, new Problem("the YAML block", error)); + } + } else { + text.append(block).append('\n'); + } + block = null; + } + continue; + } + if (block != null) { + block.append(line).append('\n'); + } else { + text.append(line).append('\n'); + } + } + if (block != null) { + // an unterminated fence: the model ran out of tokens, judge what is there as text + text.append(block); + } + for (String placeholder : placeholders(text.toString())) { + if (problems.containsKey(placeholder) || !isSimple(placeholder, catalog) + || SimpleChecks.hasPlaceholderAsLogicalOperand(placeholder)) { + continue; + } + try { + LanguageValidationResult result = catalog.validateLanguageExpression(null, "simple", placeholder); + if (!result.isSuccess()) { + String error = result.getShortError() != null ? result.getShortError() : result.getError(); + if (error != null && !SimpleChecks.isMissingDependency(error)) { + problems.put(placeholder, new Problem(placeholder, error)); + } + } + } catch (Exception e) { Review Comment: 🔍 **Broad `catch (Exception e)` — ast-grep flagged, but context matters** The comment explains the intent: "best effort — what the catalog cannot judge is not reported." That reasoning is sound for a UI feature (no need to crash the panel on a catalog quirk), and `SimpleChecks` follows the same pattern. However, completely silent swallowing means a bug in the catalog's `validateLanguageExpression` (e.g., `NullPointerException`) would also be swallowed and produce a wrong answer silently. Consider at least logging at DEBUG level so the AI log picks it up: ```suggestion } catch (Exception e) { // best effort: what the catalog cannot judge is not reported log.debug("AnswerChecks: catalog validation threw for '{}': {}", placeholder, e.getMessage()); } ``` If `AnswerChecks` has no logger, a `System.getLogger` or the Camel logging facade would work. Minor, but the AI log is exactly where this kind of diagnostic belongs. -- 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]
