Repository navigation
CAMEL-26384: camel-jbang - AI tools: a resource: in both values of a Simple ternary goes before the function, and the list item column message quotes the lines - #28634
Conversation
…Simple ternary goes before the function, and the list item column message quotes the lines - A top-level ternary whose two values start with the same resource: and scheme is rewritten with that prefix before the function: inside it, 'resource:file:order.json' is only the text the body becomes. - The list item column message quotes the item's line and the list's first line as written; the lenient repair reads the quoted form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STT6whBgK1AqsSsUKrnE8m Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Both fixes are correct and well-tested.
- Ternary prefix hoisting (
SimpleChecks.java):Matcher.lookingAt()+scheme.group()semantics are correct —group()returnsgroup(0), the full match. All edge cases are guarded: non-matching schemes stay inside the function, values that equal the scheme exactly (empty remainder) are rejected byfirst.length() > scheme.end(), and a non-emptyprefixprevents double-hoisting. - ITEM_COLUMN regex (
LenientRepairs.java): the.*?in(?:\".*?\"\))?handles embedded"through backtracking — the engine extends past inner quotes to find the"\)terminator. The?suffix preserves backward compatibility with old-format messages. quotedLine()(YamlValidator.java): bounds check is correct; preserving leading whitespace is intentional (the model needs the indentation context).
Pre-fix validation: static trace confirms the new aResourceInBothValuesOfATopLevelTernaryGoesBeforeTheFunction test requires the prefix-hoisting logic absent on main — the test would fail without the fix.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 7 of 708 tested, 8 compile-only — current: 7 all testedMaveniverse Scalpel detected 7 affected modules (current approach: 7). Skip-tests mode would test 7 modules (2 direct + 6 downstream), skip tests for 8 (generated code, meta-modules) Modules Scalpel would test (7)
Modules with tests skipped (8)
💡 Manual integration tests recommended:
All tested modules (16 modules, 2m 24s total)Total reactor time: 2m 24s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
TL;DR
Comment. Both changes are right and well guarded. The resource: prefix moves out of the ternary only when it is safe to, and the quoted lines keep the one parser of that message working. No blockers.
Blockers: None
Other findings: 1 optional suggestion.
What I checked:
- Ternary rewrite (
SimpleChecks.ternaryInsideOneFunction): the prefix is hoisted only when nothing precedes the function (prefix.isEmpty()), both values start with the sameresource:<scheme>:, and both keep a non-empty remainder. Any other shape keeps the earlier quoting, as the two counter-cases inaResourceInBothValuesOfATopLevelTernaryGoesBeforeTheFunctionshow (another scheme, a non-resource value). The result has theresource:file:${... ? '...' : '...'}form that the method's Javadoc already gives as the target. - Quoted lines in the list-item message: on main,
LenientRepairs.ITEM_COLUMNis the only code that parses this message. The new optional(?: \(".*?"\))?groups accept both forms. The lazy.*?keeps working when the quoted YAML line itself contains"), because the engine extends the match until the rest of the pattern matches.AuthoringToolsLenientTestruns the validator and then the repair end to end, so its column tests (entriesQuotesAndColumns,aListItemFarOffIsNotMoved, ...) exercise the new message format.
Suggestion (optional): quotedLine quotes the whole line. In a route with a long inline value on that line, such as a JSON body or a long simple expression, the hint grows with it, and the model reads it on every retry. Capping the quoted text (for example at 120 characters with an ellipsis) would keep the hint short. The indentation, which is what the model needs, is at the start of the line anyway.
This is a rules-and-conventions review. It does not replace static analysis tools.
Reviewed with Claude Code on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…most 120 characters of a line A long inline value (a JSON body, a long expression) made the hint grow with it, and the model reads the hint on every retry. The indentation the hint is about is at the start of the line, so the quote is cut at 120 characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
@oscerd thanks, applied in f5facae: @gnodet-bot thanks for the review. Claude Code on behalf of davsclaus |
https://issues.apache.org/jira/browse/CAMEL-26384
Two findings from the local-model benchmark (r23):
${exchangeProperty.CamelTimerCounter} == 1 ? resource:file:order.json : resource:file:order-bad-email.jsonwas rewritten with both values quoted inside the function, so the body became the textresource:file:order.json(2 of 8 runs then failed withUnrecognized token 'resource'). When both values start with the sameresource:and scheme, the prefix now goes before the function:resource:file:${exchangeProperty.CamelTimerCounter == 1 ? 'order.json' : 'order-bad-email.json'}.Tests: YamlValidatorPropertyHintTest (70), all camel-yaml-dsl-validator tests, all camel-jbang-core AI tool tests (756; AuthoringToolsLenientTest and SourceValidatorSimpleTest rerun after the review change), green.
🤖 Generated with Claude Code
https://claude.ai/code/session_01STT6whBgK1AqsSsUKrnE8m