CAMEL-24538: camel-openai - fix operator precedence so an empty userMessage does not drop the body prompt - #25939
Conversation
…essage does not drop the body prompt buildUserMessage used `userPrompt == null || userPrompt.isEmpty() && isNotEmpty(config.getUserMessage())`. Because && binds tighter than ||, this is `userPrompt == null || (userPrompt.isEmpty() && ...)`, so when the USER_MESSAGE header is absent and the configured userMessage is an empty string, userPrompt was set to that empty string and buildTextMessage then used "" instead of the message body, failing with "No input provided". Parenthesize as `(userPrompt == null || userPrompt.isEmpty()) && isNotEmpty(...)` so the configured message is only used when it is actually set, otherwise the body is used. The same precedence is corrected for the (benign) system-message twin for consistency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Review
Verified beyond the diff:
- Checked out the PR branch and ran
OpenAIEmptyUserMessageBodyPromptTest— passes. - Reverted just the
userPromptprecedence fix locally and re-ran the same test — it fails with exactly the reported symptom (IllegalArgumentException: No input provided to LLM), confirming the test genuinely exercises the bug rather than passing vacuously. Restored the fix afterward. - Confirmed JIRA CAMEL-24538 is
Bug,In Progress, correctly assigned to the author. - Checked git blame on
OpenAIProducer.java— the buggy precedence has no intentional design history; it's leftover boilerplate from several unrelated feature commits, not a deliberate choice, so this fix doesn't revert anything intentional. - Confirmed the
systemPromptcompanion fix is behavior-neutral today (masked by the downstreamObjectHelper.isNotEmpty(systemPrompt)guard before the message is added) — the PR's own reasoning for touching it "for consistency" checks out.
Suggestion (non-blocking)
The identical precedence bug still exists a few lines below the systemPrompt fix, in the developerPrompt branch (outside this diff's hunk, so I can't anchor an inline suggestion to it):
// components/camel-ai/camel-openai/.../OpenAIProducer.java, buildMessages()
if (developerPrompt == null
|| developerPrompt.isEmpty() && ObjectHelper.isNotEmpty(config.getDeveloperMessage())) {
developerPrompt = config.getDeveloperMessage();
}should become:
if ((developerPrompt == null || developerPrompt.isEmpty()) && ObjectHelper.isNotEmpty(config.getDeveloperMessage())) {
developerPrompt = config.getDeveloperMessage();
}It's currently harmless (masked by the downstream ObjectHelper.isNotEmpty(developerPrompt) guard before the message is added), same as the systemPrompt case this PR already fixed "for consistency." Since two of three identical instances are being cleaned up here, it seems like an oversight to leave this third one — worth completing in the same pass, or in a fast follow-up.
Good fix, well tested, no blocking concerns.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Spotted by Claus Ibsen on the PR. The developerPrompt guard had the same
missing parentheses as userPrompt and systemPrompt:
if (developerPrompt == null
|| developerPrompt.isEmpty() && ObjectHelper.isNotEmpty(config.getDeveloperMessage()))
A null header therefore assigned the configured message unconditionally instead
of only when one is configured. Harmless today, since the downstream
ObjectHelper.isNotEmpty(developerPrompt) guard drops a null before the message is
added, but leaving one of three identical instances unfixed was an oversight.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for checking the test actually fails without the fix — that is the part I would have wanted verified too. Taken: the if ((developerPrompt == null || developerPrompt.isEmpty())
&& ObjectHelper.isNotEmpty(config.getDeveloperMessage())) {
developerPrompt = config.getDeveloperMessage();
}You are right that it is harmless today, for the same reason Built the module and ran the OpenAI tests, plus a full reactor build from the root, both clean. The three catalog doc files that the CI bot flagged as uncommitted are the Claude Code on behalf of oscerd |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean, correct bug fix. The operator precedence issue is real — Java's && binds tighter than ||, so the original a == null || a.isEmpty() && b evaluates as a == null || (a.isEmpty() && b) instead of the intended (a == null || a.isEmpty()) && b. The parenthesisation fix is applied consistently to all three affected branches (systemPrompt, developerPrompt, userPrompt), and the new test properly exercises the primary user-visible symptom.
Good investigation noting that the systemPrompt/developerPrompt instances are currently masked by downstream ObjectHelper.isNotEmpty() guards, but fixing them is the right call — leaving one of three identical instances unfixed would be an oversight.
🔀 Backport Status
main but no backport PR was found for:
camel-4.18.x— the component exists on this branch and has the same bug
Consider creating a backport PR or adding a backport-to-camel-4.18.x label.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 27 compile-only — current: 10 all testedMaveniverse Scalpel detected 37 affected modules (current approach: 10).
|
4 participants
Issue
CAMEL-24538
Problem
OpenAIProducer.buildUserMessageresolves the user prompt with:&&binds tighter than||, so this parses asuserPrompt == null || (userPrompt.isEmpty() && configHasMessage).When the
CamelOpenAIUserMessageheader is absent (userPrompt == null) and the configureduserMessageoption is an empty string, the first branch is already true, so
userPromptis set to that emptystring.
buildTextMessagethen doesuserPrompt != null ? userPrompt : body, picks the empty string overthe message body, and the request fails with "No input provided to LLM" — the body prompt is silently
dropped.
Fix
Parenthesise as
(userPrompt == null || userPrompt.isEmpty()) && ObjectHelper.isNotEmpty(config.getUserMessage())so the configured message is only substituted when it is actually set; otherwise
userPromptstays null andthe body is used.
The same precedence appears in two more places in
buildMessages(), and both are corrected here:systemPromptdeveloperPrompt— added after review feedback from @davsclausNeither of those two is observable today: their downstream
ObjectHelper.isNotEmpty(...)guard drops a nullbefore the message is added. They are fixed because leaving one of three identical instances behind would be
an oversight, not a decision.
Testing
OpenAIEmptyUserMessageBodyPromptTestconfigures the endpoint with an emptyuserMessageand assertsthe body prompt still reaches the model. Verified it fails with "No input provided" against the
unpatched code and passes with the fix.
mvn -Psourcecheck validategreen.mvn clean install -DskipTests) green after thedeveloperPromptchange,with no regenerated artifacts left uncommitted.
Claude Code on behalf of oscerd