feat: validate placeholder-derived input names on LLM/HumanInteraction/HumanDecision
Nothing today rejects a ${{...}} placeholder whose captured name contains
spaces, symbols, or brackets: the capture group is `.*?`, so it accepts
anything between ${{ and }}. That name flows straight into an IODescriptor
with no further checks.
Adds an @AssertTrue check (same convention as the existing
isOptionsUnique()/areSkillIdsUnique() checks) on LLMBlockConfiguration.prompt,
HumanInteractiveBlockConfiguration.actionDescription and
HumanDecisionBlockConfiguration.question, requiring every placeholder name to
start with a letter and contain only letters, digits, '-', '_' or '.'.
'.' stays allowed because it's already load-bearing: LoopContainer guard
prompts reference inputs.<name>/outputs.<name>
(ExecutionsService.buildGuardTemplateValues), and colliding container-exposed
names get qualified as nodeName.ioName
(ContainerFlowInterfaceResolver.qualifyWithNodeName). Confirmed via the full
suite - the first pass of this change broke 5 Loop container tests before
'.' was added back.
'[' and ']' stay rejected on purpose, reserving that syntax for a possible
future array-input marker on placeholder names.
TemplateInputs made public (was package-private) so the three
BlockConfiguration classes, which live in a different package, can call the
new hasValidPlaceholderNames() check.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
e11ef08537
commit
6ed8fc748f
|
|
@ -5,6 +5,7 @@ import java.util.List;
|
|||
import com.fasterxml.jackson.annotation.JsonProperty;
|
||||
import com.fasterxml.jackson.annotation.JsonIgnore;
|
||||
|
||||
import it.cnr.isti.workflow.manager.blocks.factories.TemplateInputs;
|
||||
import it.cnr.isti.workflow.manager.blocks.types.HumanDecisionBlockType;
|
||||
import it.cnr.isti.workflow.manager.configurations.annotations.LongText;
|
||||
import it.cnr.isti.workflow.manager.configurations.annotations.Structural;
|
||||
|
|
@ -75,6 +76,12 @@ public class HumanDecisionBlockConfiguration extends BlockConfiguration<HumanDec
|
|||
.build();
|
||||
}
|
||||
|
||||
@AssertTrue(message = "question placeholder names must start with a letter and contain only letters, digits, '-' or '_'")
|
||||
@JsonIgnore
|
||||
boolean isQuestionPlaceholderNamesValid() {
|
||||
return TemplateInputs.hasValidPlaceholderNames(question);
|
||||
}
|
||||
|
||||
@AssertTrue(message = "options must contain unique non-blank names")
|
||||
@JsonIgnore
|
||||
boolean isOptionsUnique() {
|
||||
|
|
|
|||
|
|
@ -1,9 +1,12 @@
|
|||
package it.cnr.isti.workflow.manager.blocks.configurations;
|
||||
|
||||
import com.fasterxml.jackson.annotation.JsonIgnore;
|
||||
import com.fasterxml.jackson.annotation.JsonProperty;
|
||||
|
||||
import it.cnr.isti.workflow.manager.blocks.factories.TemplateInputs;
|
||||
import it.cnr.isti.workflow.manager.configurations.annotations.LongText;
|
||||
import it.cnr.isti.workflow.manager.blocks.types.HumanInteractionBlockType;
|
||||
import jakarta.validation.constraints.AssertTrue;
|
||||
import jakarta.validation.constraints.NotBlank;
|
||||
import lombok.Builder;
|
||||
import lombok.Data;
|
||||
|
|
@ -40,5 +43,10 @@ public class HumanInteractiveBlockConfiguration extends BlockConfiguration<Human
|
|||
return configuration;
|
||||
}
|
||||
|
||||
@AssertTrue(message = "actionDescription placeholder names must start with a letter and contain only letters, digits, '-' or '_'")
|
||||
@JsonIgnore
|
||||
boolean isActionDescriptionPlaceholderNamesValid() {
|
||||
return TemplateInputs.hasValidPlaceholderNames(actionDescription);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,8 +2,10 @@ package it.cnr.isti.workflow.manager.blocks.configurations;
|
|||
|
||||
import java.util.List;
|
||||
|
||||
import com.fasterxml.jackson.annotation.JsonIgnore;
|
||||
import com.fasterxml.jackson.annotation.JsonProperty;
|
||||
|
||||
import it.cnr.isti.workflow.manager.blocks.factories.TemplateInputs;
|
||||
import it.cnr.isti.workflow.manager.blocks.types.LLMBlockType;
|
||||
import it.cnr.isti.workflow.manager.configurations.annotations.LongText;
|
||||
import it.cnr.isti.workflow.manager.configurations.annotations.Structural;
|
||||
|
|
@ -68,6 +70,12 @@ public class LLMBlockConfiguration extends BlockConfiguration<LLMBlockType> {
|
|||
return configuration;
|
||||
}
|
||||
|
||||
@AssertTrue(message = "prompt placeholder names must start with a letter and contain only letters, digits, '-' or '_'")
|
||||
@JsonIgnore
|
||||
boolean isPromptPlaceholderNamesValid() {
|
||||
return TemplateInputs.hasValidPlaceholderNames(prompt);
|
||||
}
|
||||
|
||||
@AssertTrue(message = "skills must have unique skillId values")
|
||||
boolean areSkillIdsUnique() {
|
||||
if (skills == null || skills.isEmpty()) {
|
||||
|
|
|
|||
|
|
@ -16,10 +16,19 @@ import it.cnr.isti.workflow.manager.ios.IOType;
|
|||
* description, a decision question, ...), so a block can expose more than
|
||||
* one upstream-wired input without a separate explicit input list.
|
||||
*/
|
||||
final class TemplateInputs {
|
||||
public final class TemplateInputs {
|
||||
|
||||
private static final Pattern PLACEHOLDER = Pattern.compile("\\$\\{\\{(.*?)}}");
|
||||
|
||||
// Allows '.' since it is already a load-bearing separator for LoopContainer
|
||||
// guard template values (inputs.<name> / outputs.<name>, see
|
||||
// ExecutionsService.buildGuardTemplateValues) and for container-qualified
|
||||
// exposed names (nodeName.ioName, see
|
||||
// ContainerFlowInterfaceResolver.qualifyWithNodeName). Deliberately excludes
|
||||
// '[' and ']' so a future marker syntax on placeholder names (e.g. an
|
||||
// array-input suffix) can be introduced unambiguously later.
|
||||
private static final Pattern VALID_NAME = Pattern.compile("^[A-Za-z][A-Za-z0-9_.-]*$");
|
||||
|
||||
private TemplateInputs() {
|
||||
}
|
||||
|
||||
|
|
@ -41,4 +50,13 @@ final class TemplateInputs {
|
|||
static List<IODescriptor> toInputs(Set<String> names, IOType type, List<IOCapability> capabilities) {
|
||||
return names.stream().map(name -> IODescriptor.input(name, type, false, capabilities)).toList();
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether every {@code ${{name}}} placeholder found in the given text has
|
||||
* a name that starts with a letter and contains only letters, digits,
|
||||
* '-' or '_'.
|
||||
*/
|
||||
public static boolean hasValidPlaceholderNames(String text) {
|
||||
return extractNames(text).stream().allMatch(name -> VALID_NAME.matcher(name).matches());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -121,6 +121,75 @@ class BlockConfigurationValidationTest {
|
|||
assertTrue(validator.validate(configuration).isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void humanDecisionRejectsMalformedPlaceholderNameInQuestion() {
|
||||
HumanDecisionBlockConfiguration configuration = HumanDecisionBlockConfiguration.builder()
|
||||
.name("decision")
|
||||
.question("Proceed for ${{candidate profile}}?")
|
||||
.options(List.of(new HumanDecisionOption("yes", "Yes"), new HumanDecisionOption("no", "No")))
|
||||
.build();
|
||||
|
||||
Set<ConstraintViolation<HumanDecisionBlockConfiguration>> violations = validator.validate(configuration);
|
||||
assertTrue(violations.stream().anyMatch(v -> v.getMessage().contains("placeholder names")));
|
||||
}
|
||||
|
||||
@Test
|
||||
void humanDecisionAcceptsDottedPlaceholderNameInQuestion() {
|
||||
HumanDecisionBlockConfiguration configuration = HumanDecisionBlockConfiguration.builder()
|
||||
.name("decision")
|
||||
.question("Proceed given ${{outputs.response}}?")
|
||||
.options(List.of(new HumanDecisionOption("yes", "Yes"), new HumanDecisionOption("no", "No")))
|
||||
.build();
|
||||
|
||||
assertTrue(validator.validate(configuration).isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void llmBlockRejectsMalformedPlaceholderNameInPrompt() {
|
||||
LLMBlockConfiguration configuration = LLMBlockConfiguration.builder()
|
||||
.name("llm")
|
||||
.llmDescriptor(it.cnr.isti.workflow.manager.llms.LLMDescriptor.builder()
|
||||
.provider("testProvider").model("testModel").build())
|
||||
.prompt("Summarize ${{candidate!}}")
|
||||
.build();
|
||||
|
||||
Set<ConstraintViolation<LLMBlockConfiguration>> violations = validator.validate(configuration);
|
||||
assertTrue(violations.stream().anyMatch(v -> v.getMessage().contains("placeholder names")));
|
||||
}
|
||||
|
||||
@Test
|
||||
void llmBlockAcceptsAWellFormedPrompt() {
|
||||
LLMBlockConfiguration configuration = LLMBlockConfiguration.builder()
|
||||
.name("llm")
|
||||
.llmDescriptor(it.cnr.isti.workflow.manager.llms.LLMDescriptor.builder()
|
||||
.provider("testProvider").model("testModel").build())
|
||||
.prompt("Summarize ${{candidate-profile}}")
|
||||
.build();
|
||||
|
||||
assertTrue(validator.validate(configuration).isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void humanInteractionRejectsMalformedPlaceholderNameInActionDescription() {
|
||||
HumanInteractiveBlockConfiguration configuration = HumanInteractiveBlockConfiguration.builder()
|
||||
.name("interaction")
|
||||
.actionDescription("Review ${{candidate[]}} and decide")
|
||||
.build();
|
||||
|
||||
Set<ConstraintViolation<HumanInteractiveBlockConfiguration>> violations = validator.validate(configuration);
|
||||
assertTrue(violations.stream().anyMatch(v -> v.getMessage().contains("placeholder names")));
|
||||
}
|
||||
|
||||
@Test
|
||||
void humanInteractionAcceptsAWellFormedActionDescription() {
|
||||
HumanInteractiveBlockConfiguration configuration = HumanInteractiveBlockConfiguration.builder()
|
||||
.name("interaction")
|
||||
.actionDescription("Review ${{candidate_profile}} and decide")
|
||||
.build();
|
||||
|
||||
assertTrue(validator.validate(configuration).isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
void endBlockRequiresAnOutcomeCode() {
|
||||
EndBlockConfiguration configuration = EndBlockConfiguration.builder()
|
||||
|
|
|
|||
Loading…
Reference in New Issue