Repository navigation
CAMEL-26400: camel-jbang - AI tools: camel_eval_expression takes headers, variables, resultType and predicate - #28683
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Read the evaluator and checked the surface this touches, since eval-in-an-integration is security-adjacent. It's a clean enrichment of the existing eval tool, and importantly it doesn't change the gating.
The new ExpressionEvaluator.Request (language, expression, body, headers, variables, predicate, resultType) is a tidy way to carry the richer request, and the semantics are sensible: predicate null means "guess for simple (operator present), false otherwise", and resultType mirrors a route's resultType (language default — nodes for xpath — when null). parseValues accepting either a JSON object (values keeping their JSON type) or key=value lines is the right call, since it's what makes groovy expressions like headers.rows[0].total work against structured header values, and it matches how camel_runtime_send already takes headers.
The evaluation runs in the selected integration via the dev-console / LocalCliConnector eval action, i.e. in the operator's own running process with that process's own classpath — not in the tool runner — which is the right place for it and the trusted case.
On the part I most wanted to check: this PR does not touch the TUI's ask-first gate. The only TUI file it changes is AiPanelPromptBudgetTest (a token-budget test); McpSecurityInterceptor and the AI-panel confirmation path are untouched. So the security-model contract that the TUI asks before a code-running language like groovy is unaffected — this change makes an already-gated capability richer, it doesn't open a new ungated code path. And camel-groovy is added only at <scope>test</scope>, so it's for the eval tests, not a new runtime dependency of camel-jbang-core (production groovy eval still depends on the target integration having it).
The generated dev-console metadata (eval-language.json, the dev-consoles openapi) is regenerated to match, and the tests cover the new arguments (ExpressionEvaluatorRequestTest, EvalInTheIntegrationTest, the groovy-reads-header-rows case, and the dev-console/cli-connector eval-action tests).
CI was mid-run when I looked; worth green before merge. No blockers.
Claude Code on behalf of oscerd
|
🧪 CI tested the following changed modules:
🔁 2 tests passed only after a retry on JDK 21 (3 retried attempts) Recovered flaky tests on JDK 21 (2)
🔬 Scalpel shadow comparison — Scalpel: 18 of 709 tested, 3 compile-only — current: 18 all testedMaveniverse Scalpel detected 18 affected modules (current approach: 18). Skip-tests mode would test 18 modules (7 direct + 0 downstream), skip tests for 3 (generated code, meta-modules) Modules Scalpel would test (18)
Modules with tests skipped (3)
💡 Manual integration tests recommended:
All tested modules (45 modules, 8m 28s total)Total reactor time: 8m 28s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Looks good; CI is green. Tests and docs are present and older connectors are handled. Suggestion: in the MCP AuthoringTools (~L321-328) headers and variables are declared as Object, so they skip the McpSecurityInterceptor string checks (max length, control characters); applying the length limit to the jsonText(...) result would close that. Nit: in the core camel_eval_expression executor, Boolean.valueOf(predicate.strip()) turns e.g. yes into false silently. Note this conflicts with #28678 in ExpressionEvaluator.evaluateWith (keep this PR's signature and add the withoutCliConnector wrapper); whichever merges second needs a rebase.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ers, variables, resultType and predicate camel_eval_expression took only the expression, language, body and integration name, so it could not evaluate an expression the way the route runs it: an XPath function such as string(), local-name(), boolean() or count() failed with "Can not convert #STRING to a NodeList!" (the xpath language gives nodes unless resultType says otherwise), predicate mode was guessed for simple only, and a header or variable an earlier step sets was null. A headers argument was silently ignored, and a local XPath that worked showed the class of its node list. The tool (shared registry and MCP wrapper) now takes headers and variables (a JSON object, whose values keep their JSON type, or key=value lines as camel_runtime_send takes headers), predicate for any language (simple keeps its guess when it is absent) and resultType, a class name passed to the language as its resultType option as the reifiers pass it. Locally they are set on the scratch exchange and the value is converted to String by the context's type converter, as the integration does. In the integration the eval action carries them; EvalLanguageDevConsole gets a resultType option and LocalCliConnector passes it on and keeps the JSON type of a header or variable value. An integration before Camel 4.24 gets a list or object value as JSON text, and the answer notes that it ignores resultType. An XPath that fails with "Can not convert #STRING/#BOOLEAN/#NUMBER to a NodeList" gets a hint with the argument to pass and the route form (resultType java.lang.String, predicate=true, java.lang.Integer); in the integration, where the failed eval action is a tool error, the hint is appended to the error message. camel-jbang-core gets camel-xpath, camel-groovy and camel-jq as test dependencies, so the tests evaluate them locally without a download. The prompt budgets of the TUI AI panel are raised by the ~114 tokens of the four arguments.
…option of eval-language
… the tool error of an integration before 4.24
An integration before Camel 4.24 refuses ${header.missing ?: 10}, and a failed
eval action is a tool error (CAMEL-25398), so the rewrite that CAMEL-26401 adds
to the answer never reached the model for the integration it was meant for. It
now goes with the error, as the XPath hint does.
510c6ef to
4b1703a
Compare
…s is held to the argument length limit, and a predicate is true or false McpSecurityInterceptor checks the arguments that are strings; headers and variables sent as a JSON object are not, so their JSON is now held to the same max-argument-length. A predicate other than true or false (such as yes) is refused instead of being taken for false.
|
Claude Code on behalf of Croway Thanks for the review. Done in 2f9d51c:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The new jsonText method looks correct. One minor observation on the catch block.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…/dsl/jbang/core/commands/mcp/AuthoringTools.java Co-authored-by: Guillaume Nodet - AI Bot <gnodet+bot@gmail.com>
… an error instead of throwing Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Claude Code on behalf of Croway
CAMEL-26400
Problem
camel_eval_expressiontakes onlyexpression,language,bodyandname, so it cannot evaluate an expression theway the route does:
string(),local-name(),boolean()orcount()fails withCan not convert #STRING to a NodeList!; in a route it runs withresultType: java.lang.Stringor as a predicate;
headers.x(groovy) orvariable("x")(jq) is null, and aheadersargument is silently ignored;DTMNodeList@60b333fd), because the local path answerstoString()of the value, while the integration converts it to String.Fix
camel_eval_expression(shared tool and MCP wrapper) gets four optional arguments:headersandvariables: a JSON object, whose values keep their JSON type (so the rows of a query work for agroovy
headers.rows[0].total), orkey=valuelines ascamel_runtime_sendtakes headers (same parser). The MCPwrapper declares them as
Object, so the schema takes any JSON: a client can send an object or a string;predicate: evaluate as a predicate in any language; when absent, simple keeps its guess and the other languagesare expressions;
resultType: a class name passed to the language as itsresultTypeoption, as the reifiers pass it(
createExpression(text, new Object[] { type })); the context's class resolver resolves it, soStringandIntegerwork as in a route.ExpressionEvaluatortakes aRequestrecord. Locally the headers and variables are set on the scratch exchange,and the value is converted to String by the context's type converter (falling back to
toString()), as theintegration does, so a node list shows its XML.
{key, value}form,predicateandresultType.EvalLanguageDevConsolehas a newresultTypeoption (text and JSON), andLocalCliConnector.doActionEvalTaskpasses it on and keeps the JSON type of a header or variable value (it readevery value with
getString). An integration before Camel 4.24 gets a list or object value as JSON text, and theanswer notes that it ignores
resultType.Can not convert #STRING / #BOOLEAN / #NUMBER to a NodeListgets ahintwith theargument to pass and the route form, e.g.
this XPath returns a string, not nodes: pass resultType java.lang.String (in a route: xpath: {expression: "local-name(...)", resultType: java.lang.String});predicate=truefor a boolean,java.lang.Integerorjava.lang.Doublefor a number. In the integration the failed eval action is a tool error(CAMEL-25398), so there the hint is appended to the error message.
boolean(/*[local-name()='x'])Can not convert #BOOLEAN to a NodeList!pass predicate=truepredicate: truetruelocal-name(/*[local-name()='x']/*[1])#STRING to a NodeList!pass resultType java.lang.StringresultType: java.lang.Stringy/x/yon<x><y>1</y></x>com.sun.org.apache.xml.internal.dtm.ref.DTMNodeList@60b333fd<y>1</y>headers.totalOrdersRows[0].total_orders as long,headers: {"totalOrdersRows": [{"total_orders": 21}]}Cannot invoke method getAt() on null object(variable("requestBodyJson") | fromjson).products[],variables: {"requestBodyJson": "..."}null (null) only strings can be parsed${header.foo},headers: {"foo": "bar"}bar${header.foo},headers: "foo=bar"barTesting
New tests (each fails on
origin/main: the camel-jbang-core and MCP ones do not compile there, as the argumentsand
ExpressionEvaluator.Requestare new; the console and connector ones were run against the main code of theirmodule and fail):
ExpressionEvaluatorRequestTest(local): xpathboolean()errors with thepredicate=truehint and givestruewith
predicate=true;local-name()gives theresultTypehint andywithjava.lang.String(andString);count()gets the number hint and2withjava.lang.Integer;/x/y[1]shows<y>1</y>; an unknown resultTypesays what to pass; a predicate notes that resultType is not used; groovy reads the rows of a JSON header; jq reads a
variable with
fromjson; key=value headers and variables; invalid headers say why; the registry tool takes thefour arguments as text. camel-xpath, camel-groovy and camel-jq are test dependencies of camel-jbang-core for this.
EvalInTheIntegrationTest(in-process, through the test integration and the realEvalLanguageDevConsole): theaction carries headers and variables as
{key, value},predicateandresultType; a JSON list keeps its type;an integration of Camel 4.23.0 (status file) gets the list as JSON text and the note on resultType; a failed XPath
of the integration gets the hint in the tool error.
EvalLanguageDevConsoleTest.testEvalWithResultType:${body}of0042withjava.lang.Integer(andInteger)gives
42, in JSON and text; an unknown class fails with its name. Fails on main (0042).LocalCliConnectorEvalActionTest: the eval action passes resultType, and a list header and a number variable keeptheir type (
${header.rows.size()} ${variable.count}gives1 7). Both fail on main (0042, and aClassCastExceptionongetStringof the list).AuthoringToolsTest: the wrapper passes a JSON object header, key=value variables,predicateandresultType;jsonTextserializes an object argument.Runs (
mvnd -P apache-snapshots testovercore/camel-console,dsl/camel-cli-connector,dsl/camel-jbang/camel-jbang-core,dsl/camel-jbang/camel-jbang-mcp):EvalLanguageDevConsoleTest)LocalCliConnector*Test)commands/ai/**,common/*,AskToolsTest)AiPanel*Test,TuiToolRegistry*Test,McpFacadeConfirmTest,TuiMcpServer*Test)AiPanelPromptBudgetTest: the four arguments cost ~114 tokens in every prefix (core 5119 -> 5233, core with groups5898 -> 6011, full 10731 -> 10844, semantic core 6248 -> 6361). Main was within 2 tokens of two budgets, so the
budgets are raised with a comment.
End to end, with the MCP server runner jar built from the branch (stdio client):
camel run, selected by name): the XPath errors carry the hint, and forresultType the note that 4.23.0 ignores it;
predicate: truegivestrue; a JSON object header and key=valuevariables give
x-y; a list header arrives as JSON text;resultTypewith0042gives0042with the note.headersandvariableshas notype(the wrapper declaresObject), so a client can senda JSON object or a string; both work.
Notes for reviewers
headersandvariablesareObjectin the MCP wrapper, so the schema takes a JSON object or text. A string ischecked by
McpSecurityInterceptoras before; the JSON of an object is held to the samecamel.mcp.security.max-argument-lengthin the wrapper (review of davsclaus). Apredicateother thantrueorfalseis refused.(
[ "A", "B" ]where it was["A","B"]), and other object results can read differently than theirtoString().runs the camel-jbang-core tests on CI.
Update after the rebase on main
Rebased on main after #28678 (CAMEL-26396) and #28679 (CAMEL-26401). The conflicts in
ExpressionEvaluatorand inthe
camel_eval_expressionrow ofcamel-jbang-mcp.adockeep both sides: the scratch context still starts withoutthe CLI connector, and the null and elvis notes of CAMEL-26401 run after both paths, next to the notes of this PR.
One addition (last commit): an integration before Camel 4.24 refuses the elvis operator inside
${...}, and a failedeval action is a tool error, so the rewrite CAMEL-26401 gives (
write it after the function: ${header.missing} ?: 10)never reached the model for the integration it is meant for. It now goes with the tool error, as the XPath hint of
this PR does.
EvalInTheIntegrationTest#anElvisInsideThePlaceholderRefusedByAnOlderIntegrationGetsTheRewritefailswithout it.