oscerd commented on code in PR #27422:
URL: https://github.com/apache/camel/pull/27422#discussion_r4193615221
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -160,6 +160,10 @@ static void register(Consumer<ToolDescriptor> registry) {
.param("content", "string", "The source to validate", false)
.param("camelVersion", "string", VERSION_DESC, false)
.core(true)
+ // the same content gets the same answer: a model that
validates it again and again is stuck (CAMEL-25371)
+ .deterministicWhen("content")
Review Comment:
**With `directory`, validating the same content is not deterministic**
When `directory` is passed with `content`, the answer depends on other
files: `SourceValidator.validate` then runs `BeanDeclarations.scan`,
`validateResourceRefs`, `OpenApiVerbs.bodylessEndpoints` and
`EndpointConsumerChecks.validateYamlConsumers`, which read the sibling files.
`ToolDescriptor#deterministic` itself says that "a tool that reads ... the
files ... is not" deterministic.
A concrete case on `camel tui --mcp`:
1. Validate `orders.camel.yaml` content with `to: direct:audit`, with
`directory`. The answer says no route consumes `direct:audit`.
2. Validate it again: the full answer again.
3. Do what the error says: write `audit.camel.yaml` with `from:
direct:audit` (the write path allows the missing consumer, CAMEL-24955).
4. Validate the unchanged `orders` content again. `repeatOf` answers with
the note before the tool runs ("Its answer does not change ... change the line
the error names"), although the real answer is now `valid: true`, and the line
it points at is correct.
The same applies after declaring a missing bean in a sibling file or writing
a referenced resource. Possible fixes: treat the call as deterministic only
when `content` is given and `directory` is not, forget the counts when a
non-read-only tool runs, or put a fingerprint of the directory into the key. A
test along the lines of steps 1-4 would pin it down.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/RepeatedToolCalls.java:
##########
@@ -51,7 +51,7 @@ public final class RepeatedToolCalls {
* identical calls.
*/
public synchronized JsonObject repeatOf(ToolDescriptor tool, Map<String,
?> args) {
- if (tool == null || !tool.isDeterministic()) {
+ if (tool == null || !tool.isDeterministic(args)) {
Review Comment:
**The repeat key trims `content`, so a whitespace-only fix counts as the
same call**
`key()` trims every value. That suits short catalog arguments, but now it
also applies to `content`, where leading/trailing whitespace can decide the
result: XML with a newline before `<?xml ...?>` is rejected by
`XmlChecks.validateXml` ("The processing instruction target matching
"[xX][mM][lL]" is not allowed"), while the same text without the newline is
valid. Once the first form has been validated twice, the fix (removing the
newline) gets the repeat note instead of a validation, because both trim to the
same key. The key also now holds the whole source text, for up to `MAX_TRACKED`
entries per session. Keying `content` on a digest of the exact text would fix
both and still match the byte-identical repeats the benchmark found.
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/AuthoringTools.java:
##########
@@ -174,8 +178,16 @@ static void register(Consumer<ToolDescriptor> registry) {
throw new ToolExecutionException("No such file in
the directory: " + file);
}
content = read(path);
+ return validate(ctx, dir, file, content).toJson();
}
- return validate(ctx, dir, file, content).toJson();
+ JsonObject result = validate(ctx, dir, file, content);
+ if (sameAsOnDisk(ctx, directory, file, content)) {
+ // a model that meant to change the file validates the
old version: say so (CAMEL-25371)
+ result.put("sameAsFile", true);
+ result.put("note", "This content is the same as " +
file + " on disk: an edit you meant to make"
Review Comment:
**The `sameAsFile` note states a missing edit as fact**
The note states a missing edit as a fact, which misleads in two common
flows: after `camel_write_file` answers with `existingProblems` and the model
validates the same content to look at them, and when the model reads the file
with `camel_get_files` and passes that text as `content` to explain a reload
error. A small model may then believe its write failed and write again. A
conditional wording keeps the hint for the stuck case, e.g. "This content is
the same as <file> on disk: if you meant to change the file, that change is not
in this content."
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/ai/ToolDescriptor.java:
##########
@@ -128,6 +145,21 @@ public boolean isDeterministic() {
return deterministic;
}
+ /**
+ * Whether this call gives the same answer each time: the tool is
deterministic, or the argument that makes it so.
+ */
+ public boolean isDeterministic(java.util.Map<String, ?> args) {
Review Comment:
**FQCN where the import already exists**
Minor: `java.util.Map` is already imported in this file, and CLAUDE.md asks
for imports instead of fully qualified names (the same applies to
`@org.junit.jupiter.api.io.TempDir java.nio.file.Path` /
`java.nio.file.Files.writeString` in the new `AuthoringToolsTest` case, where
those types are already imported).
```suggestion
public boolean isDeterministic(Map<String, ?> args) {
```
--
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]
