Skip to content

fix: validate Bedrock conversation messages #857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fdelbrayelle merged 1 commit into from
Oct 23, 2026
Merged

fix: validate Bedrock conversation messages #857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/main/java/io/kestra/plugin/aws/bedrock/Converse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import io.kestra.plugin.aws.shared.ConnectionUtils;

import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotEmpty;
import jakarta.validation.constraints.NotNull;
import lombok.*;
import lombok.experimental.SuperBuilder;
Expand Down Expand Up @@ -84,7 +83,7 @@ public class Converse extends AbstractConnection implements RunnableTask<Convers
description = "Ordered list of conversation turns. Each entry must have a `role` (`user` or `assistant`) and `content` (text string)."
)
@PluginProperty(group = "main")
@NotEmpty
@NotNull
private Property<List<Map<String, String>>> messages;

@Schema(
Expand Down Expand Up @@ -115,6 +114,9 @@ public Output run(RunContext runContext) throws Exception {
} catch (ClassCastException e) {
throw new IllegalArgumentException("messages must be a list of {role, content} maps", e);
}
if (rawMessages.isEmpty()) {

@fdelbrayelle fdelbrayelle Oct 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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 not be empty");

@fdelbrayelle fdelbrayelle Oct 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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.

}

var sdkMessages = BedrockUtils.buildMessages(rawMessages);

Expand Down
6 changes: 4 additions & 2 deletions src/main/java/io/kestra/plugin/aws/bedrock/ConverseStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.kestra.plugin.aws.shared.ConnectionUtils;

import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotEmpty;
import jakarta.validation.constraints.NotNull;
import lombok.*;
import lombok.experimental.SuperBuilder;
Expand Down Expand Up @@ -87,7 +86,7 @@ public class ConverseStream extends AbstractConnection implements RunnableTask<C
description = "Ordered list of conversation turns. Each entry must have a `role` (`user` or `assistant`) and `content` (text string)."
)
@PluginProperty(group = "main")
@NotEmpty
@NotNull
private Property<List<Map<String, String>>> messages;

@Schema(
Expand Down Expand Up @@ -118,6 +117,9 @@ public Output run(RunContext runContext) throws Exception {
} catch (ClassCastException e) {
throw new IllegalArgumentException("messages must be a list of {role, content} maps", e);
}
if (rawMessages.isEmpty()) {
throw new IllegalArgumentException("messages must not be empty");
}

var sdkMessages = BedrockUtils.buildMessages(rawMessages);

Expand Down
49 changes: 49 additions & 0 deletions src/test/java/io/kestra/plugin/aws/bedrock/ConverseStreamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import io.kestra.core.runners.RunContextFactory;

import jakarta.inject.Inject;
import jakarta.validation.MessageInterpolator;
import jakarta.validation.Validation;
import software.amazon.awssdk.services.bedrockruntime.BedrockRuntimeAsyncClient;
import software.amazon.awssdk.services.bedrockruntime.model.*;

Expand Down Expand Up @@ -129,4 +131,51 @@ void givenInvalidRole_whenConverseStream_thenThrowsIllegalArgumentException() th

assertThrows(IllegalArgumentException.class, () -> spy.run(runContext));
}

@Test
void givenEmptyMessages_whenConverseStream_thenThrowsExpectedException() {
RunContext runContext = runContextFactory.of();

ConverseStream task = ConverseStream.builder()
.id("test-converse-stream-empty-messages")
.type(ConverseStream.class.getName())
.region(Property.ofValue("us-east-1"))
.modelId(Property.ofValue("anthropic.claude-3-5-sonnet-20241022-v2:0"))
.messages(Property.ofValue(List.of()))
.build();

var exception = assertThrows(IllegalArgumentException.class, () -> task.run(runContext));

assertThat(exception.getMessage(), is("messages must not be empty"));
}

@Test
void givenValidMessages_whenValidated_thenDoesNotProduceHv000030() {
ConverseStream task = ConverseStream.builder()
.id("test-converse-stream-validation")
.type(ConverseStream.class.getName())
.region(Property.ofValue("us-east-1"))
.modelId(Property.ofValue("anthropic.claude-3-5-sonnet-20241022-v2:0"))
.messages(Property.ofValue(List.of(Map.of("role", "user", "content", "Hello!"))))
.build();

var violations = Validation.byDefaultProvider()
.configure()
.messageInterpolator(new MessageInterpolator() {
@Override
public String interpolate(String messageTemplate, Context context) {
return messageTemplate;
}

@Override
public String interpolate(String messageTemplate, Context context, java.util.Locale locale) {
return messageTemplate;
}
})
.buildValidatorFactory()
.getValidator()
.validate(task);

assertThat(violations, empty());
}
}
50 changes: 50 additions & 0 deletions src/test/java/io/kestra/plugin/aws/bedrock/ConverseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
import io.kestra.core.runners.RunContextFactory;

import jakarta.inject.Inject;
import jakarta.validation.MessageInterpolator;
import jakarta.validation.Validation;
import software.amazon.awssdk.services.bedrockruntime.BedrockRuntimeClient;
import software.amazon.awssdk.services.bedrockruntime.model.*;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.*;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;

Expand Down Expand Up @@ -77,4 +80,51 @@ void givenValidMessages_whenConverse_thenOutputContainsContent() throws Exceptio
assertThat(output.getInputTokens(), is(10));
assertThat(output.getOutputTokens(), is(15));
}

@Test
void givenEmptyMessages_whenConverse_thenThrowsExpectedException() {
RunContext runContext = runContextFactory.of();

@fdelbrayelle fdelbrayelle Oct 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 [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.


Converse task = Converse.builder()
.id("test-converse-empty-messages")
.type(Converse.class.getName())
.region(Property.ofValue("us-east-1"))
.modelId(Property.ofValue("anthropic.claude-3-5-sonnet-20241022-v2:0"))
.messages(Property.ofValue(List.of()))
.build();

var exception = assertThrows(IllegalArgumentException.class, () -> task.run(runContext));

assertThat(exception.getMessage(), is("messages must not be empty"));
}

@Test
void givenValidMessages_whenValidated_thenDoesNotProduceHv000030() {
Converse task = Converse.builder()
.id("test-converse-validation")
.type(Converse.class.getName())
.region(Property.ofValue("us-east-1"))
.modelId(Property.ofValue("anthropic.claude-3-5-sonnet-20241022-v2:0"))
.messages(Property.ofValue(List.of(Map.of("role", "user", "content", "Hello!"))))
.build();

var violations = Validation.byDefaultProvider()

@fdelbrayelle fdelbrayelle Oct 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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.

.configure()
.messageInterpolator(new MessageInterpolator() {
@Override
public String interpolate(String messageTemplate, Context context) {
return messageTemplate;
}

@Override
public String interpolate(String messageTemplate, Context context, java.util.Locale locale) {

@fdelbrayelle fdelbrayelle Oct 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 [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.

return messageTemplate;
}
})
.buildValidatorFactory()
.getValidator()
.validate(task);

assertThat(violations, empty());
}
}
Loading