From 6ed8fc748f84d91ca7659553b01dfd38ad06ed64 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Fri, 24 Jul 2026 09:55:24 +0200 Subject: [PATCH] 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./outputs. (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 --- .../HumanDecisionBlockConfiguration.java | 7 ++ .../HumanInteractiveBlockConfiguration.java | 8 +++ .../configurations/LLMBlockConfiguration.java | 8 +++ .../blocks/factories/TemplateInputs.java | 20 +++++- .../BlockConfigurationValidationTest.java | 69 +++++++++++++++++++ 5 files changed, 111 insertions(+), 1 deletion(-) diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/HumanDecisionBlockConfiguration.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/HumanDecisionBlockConfiguration.java index 14034bb..bf77b8f 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/HumanDecisionBlockConfiguration.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/HumanDecisionBlockConfiguration.java @@ -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 { 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()) { diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/factories/TemplateInputs.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/factories/TemplateInputs.java index 68957c8..fcf0353 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/factories/TemplateInputs.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/factories/TemplateInputs.java @@ -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. / outputs., 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 toInputs(Set names, IOType type, List 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()); + } } diff --git a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/BlockConfigurationValidationTest.java b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/BlockConfigurationValidationTest.java index 9b73130..64cdd84 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/BlockConfigurationValidationTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/BlockConfigurationValidationTest.java @@ -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> 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> 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> 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()