Repository navigation
fix: validate Bedrock conversation messages - #857
Conversation
fdelbrayelle
left a comment
There was a problem hiding this comment.
Kestra Plugin Code Review
Business Requirements: met
Fixes #819: @NotEmpty is removed from messages in both Converse and ConverseStream (the only two Property fields in the repo carrying it), replaced by @NotNull plus an empty check after rendering, as the issue suggests. The new validation tests fail with HV000030 against the main code and pass on this head, so they guard the regression.
Kestra Guidelines: 5 findings
- 🟡
Converse.java:117[Guidelines] Empty check duplicated in both tasks - 🟡
Converse.java:118[Guidelines] Error message does not say what to do - 🟡
ConverseTest.java:111[Tests] Hand-rolled validator instead ofModelValidator - 🟢
ConverseTest.java:120[Guidelines] Fully qualifiedjava.util.Locale - 🟢
ConverseTest.java:86[Guidelines] Explicit local types and fixed task ids
Security (OWASP Top 10:2025 + KPS): 0 findings
✅ No issues found
Performance: 0 findings
✅ No issues found
Tests: ConverseTest and ConverseStreamTest run locally on 90a41a1, 7 passed, 0 failed. CI green.
Confidence: 83% (100 - 5 Suggestion - 5 Suggestion - 5 Suggestion - 1 Nit - 1 Nit)
Verdict: APPROVE; recommendation only, posted as COMMENT, a maintainer decides.
| } catch (ClassCastException e) { | ||
| throw new IllegalArgumentException("messages must be a list of {role, content} maps", e); | ||
| } | ||
| if (rawMessages.isEmpty()) { |
There was a problem hiding this comment.
🟡 [Guidelines] Empty check duplicated in both tasks
Problem: The same rawMessages.isEmpty() guard is copied into Converse and ConverseStream (line 120), so the two can drift apart; the guidelines ask for shared behaviour to be extracted (DRY).
Fix: Move the check to the top of BedrockUtils.buildMessages(...), which both tasks already call with the rendered list, and drop it from the two run() methods.
| throw new IllegalArgumentException("messages must be a list of {role, content} maps", e); | ||
| } | ||
| if (rawMessages.isEmpty()) { | ||
| throw new IllegalArgumentException("messages must not be empty"); |
There was a problem hiding this comment.
🟡 [Guidelines] Error message does not say what to do
Problem: messages must not be empty states the failure but not the fix, and since @NotEmpty is gone the property docs no longer signal that at least one entry is required (Error Messages: say what went wrong AND what to do).
Fix: Use something like "messages must contain at least one {role, content} entry, for example a 'user' turn", and add "At least one entry is required." to the messages @Schema description in both tasks.
| .messages(Property.ofValue(List.of(Map.of("role", "user", "content", "Hello!")))) | ||
| .build(); | ||
|
|
||
| var violations = Validation.byDefaultProvider() |
There was a problem hiding this comment.
🟡 [Tests] Hand-rolled validator instead of ModelValidator
Problem: The test builds its own Validation.byDefaultProvider() with an anonymous MessageInterpolator, duplicated in ConverseStreamTest, rather than the validator Kestra uses when a flow is saved.
Fix: @Inject ModelValidator modelValidator; (io.kestra.core.models.validations.ModelValidator) and assert assertThat(modelValidator.isValid(task).isEmpty(), is(true)); in both test classes; this also removes the interpolator boilerplate.
| } | ||
|
|
||
| @Override | ||
| public String interpolate(String messageTemplate, Context context, java.util.Locale locale) { |
There was a problem hiding this comment.
🟢 [Guidelines] Fully qualified java.util.Locale
Problem: java.util.Locale is written inline here and in ConverseStreamTest.java:171 with no naming conflict (Java Style: import classes instead of using fully qualified names).
Fix: Add import java.util.Locale; in both test files, or drop the anonymous interpolator entirely if you switch to ModelValidator.
|
|
||
| @Test | ||
| void givenEmptyMessages_whenConverse_thenThrowsExpectedException() { | ||
| RunContext runContext = runContextFactory.of(); |
There was a problem hiding this comment.
🟢 [Guidelines] Explicit local types and fixed task ids
Problem: New tests declare RunContext runContext and Converse task / ConverseStream task instead of var, and use fixed ids like test-converse-empty-messages (Java Style: var for locals; Flaky Test Prevention: randomize task ids). Same in ConverseStreamTest.
Fix: Use var runContext = ... and var task = ..., and build ids with IdUtils.create() or a random suffix.
QA reportCommit: 90a41a1 Setup: Bedrock calls go to a local mock Bedrock runtime ( Summary
Per-flow results (identical on both versions)All four templated and literal success runs: task Empty list runs: Save step: the flows save with Flow: tpl_converseFlow YAML: tpl_converseid: tpl_converse
namespace: qa.aws857
inputs:
- id: messages
type: JSON
defaults: '[{"role":"user","content":"Hello"}]'
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.Converse
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages: "{{ inputs.messages }}"Flow: tpl_streamFlow YAML: tpl_streamid: tpl_stream
namespace: qa.aws857
inputs:
- id: messages
type: JSON
defaults: '[{"role":"user","content":"Hello"}]'
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.ConverseStream
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages: "{{ inputs.messages }}"Flow: tpl_converse_emptyFlow YAML: tpl_converse_emptyid: tpl_converse_empty
namespace: qa.aws857
inputs:
- id: messages
type: JSON
defaults: '[]'
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.Converse
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages: "{{ inputs.messages }}"Flow: tpl_stream_emptyFlow YAML: tpl_stream_emptyid: tpl_stream_empty
namespace: qa.aws857
inputs:
- id: messages
type: JSON
defaults: '[]'
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.ConverseStream
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages: "{{ inputs.messages }}"Flow: lit_converseFlow YAML: lit_converseid: lit_converse
namespace: qa.aws857
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.Converse
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages:
- role: user
content: HelloFlow: lit_streamFlow YAML: lit_streamid: lit_stream
namespace: qa.aws857
tasks:
- id: chat
type: io.kestra.plugin.aws.bedrock.ConverseStream
region: us-east-1
accessKeyId: test
secretKeyId: test
endpointOverride: http://qa-aws857-mock:8080
modelId: anthropic.claude-3-haiku-20240307-v1:0
messages:
- role: user
content: HelloNotes
|
What changes are being made and why?
Fixes #819
This PR fixes the
HV000030validation error in the BedrockConverseandConverseStreamtasks.The
messagesfield is a KestraProperty<List<Map<String, String>>>. Using@NotEmptydirectly on thePropertycauses Hibernate Validator to fail because@NotEmptyis not supported for this type.Changes:
@NotEmptywith@NotNullonmessages.ConverseandConverseStreamcovering empty and valid messages.HV000030error.How the changes have been QAed?
The focused tests were executed successfully:
The tests cover:
HV000030validation error no longer occurring.ConverseandConverseStreambehavior remaining functional.Example flow:
Setup Instructions
No additional setup requirements beyond the existing AWS Bedrock configuration for the plugin.
Contributor Checklist ✅