From 7d3f6b088ca01dc1b0cb9104b9b86f12b7096f0e Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Tue, 8 Sep 2026 10:52:52 +0200 Subject: [PATCH] Resolve every configurable-as-input field through one rule Three fields are declared @ConfigurableAsInput - the model of an LLM descriptor and the two on the MCP blocks - and the editor offers the same choices on all of them: leave it blank and feed the port, or write a template such as ${{global.modelName}}. It was resolved three times and differently. The MCP executors each carried an identical private copy that fell back to the configured value raw, so a placeholder written there reached the MCP service verbatim, and a global input could not decide the model of an MCP block at all. The rule now lives in ConfigurableInputBinding: a value bound to the port wins, otherwise the configured value is resolved as a template against the same inputs and variables a prompt is. LLMDescriptorInputBinding keeps only what is specific to it - rebuilding the record around the resolved model, and resolving the model alone, since the provider decides the credential and the sampling parameters and cannot arrive mid-execution. The MCP model fields carry @AcceptsVariablePlaceholder to match, so the editor declares what the runtime has always been asked to accept. Co-Authored-By: Claude Opus 5 (1M context) --- .../MCPAgentBlockConfiguration.java | 2 + .../MCPAgentChatBlockConfiguration.java | 2 + .../blocks/MCPAgentChatExecutor.java | 17 ++-- .../executors/blocks/MCPBridgeExecutor.java | 13 +-- .../llms/ConfigurableInputBinding.java | 77 +++++++++++++++++ .../llms/LLMDescriptorInputBinding.java | 58 +++---------- .../controllers/BlocksControllerTest.java | 14 ++++ .../manager/executions/ExecutionTest.java | 39 +++++++++ .../llms/ConfigurableInputBindingTest.java | 82 +++++++++++++++++++ 9 files changed, 234 insertions(+), 70 deletions(-) create mode 100644 src/main/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBinding.java create mode 100644 src/test/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBindingTest.java diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentBlockConfiguration.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentBlockConfiguration.java index aeac0c4..dab66b7 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentBlockConfiguration.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentBlockConfiguration.java @@ -11,6 +11,7 @@ import it.cnr.isti.workflow.manager.configurations.annotations.FieldRetriever; import it.cnr.isti.workflow.manager.configurations.annotations.LongText; import it.cnr.isti.workflow.manager.configurations.annotations.Structural; import it.cnr.isti.workflow.manager.configurations.annotations.DynamicSchema; +import it.cnr.isti.workflow.manager.configurations.annotations.AcceptsVariablePlaceholder; import it.cnr.isti.workflow.manager.configurations.annotations.ConfigurableAsInput; import it.cnr.isti.workflow.manager.configurations.annotations.UiContextKeys; import it.cnr.isti.workflow.manager.configurations.annotations.SchemaAllowedValues; @@ -35,6 +36,7 @@ public class MCPAgentBlockConfiguration extends BlockConfiguration inputs) { - return inputs.stream() - .filter(input -> inputName.equals(input.getDescriptor().getName())) - .findFirst() - .map(Input::getValue) - .map(Object::toString) - .filter(StringUtils::hasText) - .orElse(configuredValue); - } - private List mapServers( List servers) { if (servers == null || servers.isEmpty()) { diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPBridgeExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPBridgeExecutor.java index 7dd7b95..5d9e8b1 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPBridgeExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPBridgeExecutor.java @@ -17,6 +17,7 @@ import it.cnr.isti.workflow.manager.executions.ExecutionVariableDescriptor; import it.cnr.isti.workflow.manager.executions.ExecutionTemplateResolver; import it.cnr.isti.workflow.manager.executions.steps.Input; import it.cnr.isti.workflow.manager.executions.bias.runtime.BiasRuntimeSupport; +import it.cnr.isti.workflow.manager.llms.ConfigurableInputBinding; import it.cnr.isti.workflow.manager.mcp.MCPAgentService; import it.cnr.isti.workflow.manager.mcp.MCPSharedSessionRegistry; @@ -35,7 +36,7 @@ public class MCPBridgeExecutor implements BlockExecutor { prompt = BiasRuntimeSupport.decoratePrompt(prompt, executionVariables); String model = Boolean.TRUE.equals(config.getUseSharedSession()) ? null - : resolveConfigurableInput("model", config.getModel(), inputs); + : ConfigurableInputBinding.resolve("model", config.getModel(), inputs, executionVariables); String configuredSharedSessionKey = normalize(Boolean.TRUE.equals(config.getUseSharedSession()) ? config.getSharedSessionRef() : null); @@ -111,16 +112,6 @@ public class MCPBridgeExecutor implements BlockExecutor { return ExecutionTemplateResolver.resolve(template, inputs, executionVariables); } - private String resolveConfigurableInput(String inputName, String configuredValue, List inputs) { - return inputs.stream() - .filter(input -> inputName.equals(input.getDescriptor().getName())) - .findFirst() - .map(Input::getValue) - .map(Object::toString) - .filter(StringUtils::hasText) - .orElse(configuredValue); - } - private String normalize(String value) { return StringUtils.hasText(value) ? value.trim() : null; } diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBinding.java b/src/main/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBinding.java new file mode 100644 index 0000000..ccc59c0 --- /dev/null +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBinding.java @@ -0,0 +1,77 @@ +package it.cnr.isti.workflow.manager.llms; + +import java.util.List; +import java.util.Map; + +import org.springframework.util.StringUtils; + +import it.cnr.isti.workflow.manager.executions.ExecutionTemplateResolver; +import it.cnr.isti.workflow.manager.executions.steps.Input; + +/** + * The effective value of a {@code @ConfigurableAsInput} field: what the block should actually use, + * given that the field may not name it outright. + * + *

Two ways to not name it, and they are mutually exclusive by construction: + *

    + *
  • left blank, so the block factory turned it into an input port, fed by a connection or by a + * value provided when the run is launched;
    • + *
  • written as a template - {@code ${{global.modelName}}}, or any input's name - resolved + * against the same values as a prompt. Global inputs reach a block only through template + * interpolation, never by feeding a port, so this is the only way one can decide such a + * field.
    • + *
+ * + *

One place for the rule because there are now three such fields - the model of an + * {@link LLMDescriptor} and the two on the MCP blocks - and the editor offers the same choices on + * all of them. It used to be resolved three times and differently: the MCP executors each carried + * an identical private copy that returned the configured value raw, so a placeholder written there + * would have reached the MCP service verbatim. + */ +public final class ConfigurableInputBinding { + + private ConfigurableInputBinding() { + } + + /** + * @param inputName the port's name, as declared by {@code @ConfigurableAsInput} + * @param configuredValue what the configuration holds, possibly a template, possibly blank + */ + public static String resolve(String inputName, String configuredValue, List inputs, + Map executionVariables) { + String bound = fromInputs(inputName, inputs); + if (StringUtils.hasText(bound)) { + return bound; + } + return ExecutionTemplateResolver.resolve(configuredValue, inputs, executionVariables); + } + + /** + * The same rule for the executors that have already reduced their inputs to a name-to-value map + * (Conditional and Switch, which evaluate SpEL over it): asking them to carry the raw input list + * down into a private helper only to read one entry would be worse. + */ + public static String resolve(String inputName, String configuredValue, Map inputValues, + Map executionVariables) { + Object bound = inputValues == null ? null : inputValues.get(inputName); + if (bound != null && StringUtils.hasText(bound.toString())) { + return bound.toString(); + } + return ExecutionTemplateResolver.resolve(configuredValue, inputValues, executionVariables); + } + + private static String fromInputs(String inputName, List inputs) { + if (inputs == null || inputName == null) { + return null; + } + return inputs.stream() + .filter(input -> input != null && input.getDescriptor() != null + && inputName.equals(input.getDescriptor().getName())) + .map(Input::getValue) + .filter(value -> value != null) + .map(Object::toString) + .filter(StringUtils::hasText) + .findFirst() + .orElse(null); + } +} diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMDescriptorInputBinding.java b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMDescriptorInputBinding.java index e9f5201..490ba9c 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMDescriptorInputBinding.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMDescriptorInputBinding.java @@ -2,27 +2,18 @@ package it.cnr.isti.workflow.manager.llms; import java.util.List; import java.util.Map; +import java.util.Objects; -import org.springframework.util.StringUtils; - -import it.cnr.isti.workflow.manager.executions.ExecutionTemplateResolver; import it.cnr.isti.workflow.manager.executions.steps.Input; /** - * Resolves the model a block is actually going to call, which it may not have configured outright. + * {@link ConfigurableInputBinding} applied to the model of an {@link LLMDescriptor}, rebuilding the + * descriptor around the result - a record, so it cannot be modified in place. * - *

Two ways to not name it directly, and they compose: - *

    - *
  • left blank, so {@code @ConfigurableAsInput} turned it into a {@code model} port, fed by a - * connection or by a value provided when the run is launched;
    • - *
  • written as a template - {@code ${{global.modelName}}}, or any input's name - resolved the - * same way and against the same values as a prompt. Global inputs reach a block only through - * template interpolation, so this is the only way one can decide the model.
    • - *
- * - *

Only the model. The provider stays configured because it is what decides the credential - - * {@code AuthorizationRequirementResolver} reads it before the run starts - and which sampling - * parameters the call can honour; neither could be answered if it arrived mid-execution. + *

Only the model is resolved this way. The provider stays configured because it is what decides + * the credential - {@code AuthorizationRequirementResolver} reads it before the run starts - and + * which sampling parameters the call can honour; neither could be answered if it arrived + * mid-execution. * *

Every executor that reads a descriptor from its configuration goes through here, so the rule * lives in one place rather than being re-derived per block. @@ -40,29 +31,17 @@ public final class LLMDescriptorInputBinding { if (configured == null) { return null; } - String bound = modelFromInputs(inputs); - if (StringUtils.hasText(bound)) { - return withModel(configured, bound); - } - return withModel(configured, ExecutionTemplateResolver.resolve(configured.model(), inputs, executionVariables)); + return withModel(configured, + ConfigurableInputBinding.resolve(MODEL_INPUT, configured.model(), inputs, executionVariables)); } - /** - * The same rule for the executors that have already reduced their inputs to a name-to-value - * map (Conditional and Switch, which evaluate SpEL over it): asking them to carry the raw - * input list down into a private helper only to read one entry would be worse. - */ public static LLMDescriptor resolve(LLMDescriptor configured, Map inputValues, Map executionVariables) { if (configured == null) { return null; } - Object bound = inputValues == null ? null : inputValues.get(MODEL_INPUT); - if (bound != null && StringUtils.hasText(bound.toString())) { - return withModel(configured, bound.toString()); - } return withModel(configured, - ExecutionTemplateResolver.resolve(configured.model(), inputValues, executionVariables)); + ConfigurableInputBinding.resolve(MODEL_INPUT, configured.model(), inputValues, executionVariables)); } /** @@ -70,7 +49,7 @@ public final class LLMDescriptorInputBinding { * overwhelmingly common case - costs nothing and stays identical rather than merely equal. */ private static LLMDescriptor withModel(LLMDescriptor configured, String model) { - if (java.util.Objects.equals(configured.model(), model)) { + if (Objects.equals(configured.model(), model)) { return configured; } return LLMDescriptor.builder() @@ -79,19 +58,4 @@ public final class LLMDescriptorInputBinding { .parameters(configured.parameters()) .build(); } - - private static String modelFromInputs(List inputs) { - if (inputs == null) { - return null; - } - return inputs.stream() - .filter(input -> input != null && input.getDescriptor() != null - && MODEL_INPUT.equals(input.getDescriptor().getName())) - .map(Input::getValue) - .filter(value -> value != null) - .map(Object::toString) - .filter(StringUtils::hasText) - .findFirst() - .orElse(null); - } } diff --git a/src/test/java/it/cnr/isti/workflow/manager/controllers/BlocksControllerTest.java b/src/test/java/it/cnr/isti/workflow/manager/controllers/BlocksControllerTest.java index e3a0a3b..f5eaecf 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/controllers/BlocksControllerTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/controllers/BlocksControllerTest.java @@ -141,6 +141,20 @@ public class BlocksControllerTest { assertFalse(descriptor.path("properties").path("provider") .path("x-ui-accept-variable-as-placeholder").asBoolean()); + // The same on the MCP blocks' own model. The editor offers the same three Source choices + // wherever a field is configurable as an input, so every one of them has to be resolved as + // a template - and the schema is what says so. + for (String blockType : List.of(MCPAgentBlockType.TYPE, MCPAgentChatBlockType.TYPE)) { + JsonNode mcpModel = (JsonNode) catalog.descriptors().stream() + .filter(type -> blockType.equals(type.type())) + .findFirst() + .orElseThrow() + .schema(); + JsonNode modelProperty = mcpModel.path("properties").path("model"); + assertTrue(modelProperty.path("x-ui-bindable-as-input").asBoolean(), blockType); + assertTrue(modelProperty.path("x-ui-accept-variable-as-placeholder").asBoolean(), blockType); + } + // The parameters themselves: present, optional, and carrying their range so the editor can // bound the input rather than accepting anything and failing on save. JsonNode parameters = catalog.sharedDefinitions().path("ModelParameters"); diff --git a/src/test/java/it/cnr/isti/workflow/manager/executions/ExecutionTest.java b/src/test/java/it/cnr/isti/workflow/manager/executions/ExecutionTest.java index 2136318..4afb489 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/executions/ExecutionTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/executions/ExecutionTest.java @@ -1207,6 +1207,45 @@ public class ExecutionTest { Mockito.verify(mcpAgentService).querySession("session-resume", "Continue with John Doe"); } + @Test + public void mcpAgentExecutionResolvesAGlobalInputWrittenIntoTheModelField() { + // The case that would have broken: the MCP executors used to return the configured value + // raw, so a placeholder written into the model reached the MCP service verbatim. The mock + // is keyed on the resolved name, so an unresolved one fails here rather than silently. + Block block = mcpAgentBlockFactory.create(MCPAgentBlockConfiguration.builder() + .name("Research") + .model("${{global.modelName}}") + .prompt("Find data for ${{cand}}") + .build()); + + // The field is written, not blank, so no port is offered for it. + assertTrue(block.getInputs().stream().noneMatch(input -> input.getName().equals("model"))); + + FlowData flow = FlowData.builder() + .block(block) + .globalInput(IODescriptor.input("modelName", IOType.TEXT, false, null)) + .build(); + + // Without a shared session an MCPAgent goes straight through execute(), and the stub is + // keyed on the resolved model: an unresolved placeholder finds no match and fails here. + Mockito.when(mcpAgentService.execute(Mockito.eq("llama3.1:8b"), Mockito.eq("Find data for Ada"), + Mockito.any(), Mockito.anyMap())).thenReturn("Ada data"); + + ExecutionObject execObject = executionsService.createExecution("MCP global model flow", flow); + execObject = executionsService.setGlobalInput(execObject.getId(), "modelName", "llama3.1:8b"); + executionsService.prepareInput(execObject.getId(), block.getId(), "cand", "Ada"); + execObject = executionsService.startExecution(execObject.getId()); + while (execObject.getContext().getStatus() == ExecutionStatus.RUNNING) { + execObject = executionsService.getExecution(execObject.getId()); + } + + assertEquals(ExecutionStatus.SUCCESS, execObject.getContext().getStatus()); + Mockito.verify(mcpAgentService).execute(Mockito.eq("llama3.1:8b"), Mockito.eq("Find data for Ada"), + Mockito.any(), Mockito.anyMap()); + assertEquals("Ada data", execObject.getContext().getResult() + .get(new FieldKey(block.getId(), MCPAgentBlockFactory.OUTPUT_NAME))); + } + @Test public void mcpAgentBlockCanShareSessionWithFollowingMcpAgentBlock() { Block producer = mcpAgentBlockFactory.create(MCPAgentBlockConfiguration.builder() diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBindingTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBindingTest.java new file mode 100644 index 0000000..b9a2ae0 --- /dev/null +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/ConfigurableInputBindingTest.java @@ -0,0 +1,82 @@ +package it.cnr.isti.workflow.manager.llms; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import it.cnr.isti.workflow.manager.blocks.IOCapability; +import it.cnr.isti.workflow.manager.blocks.IOCapabilityType; +import it.cnr.isti.workflow.manager.executions.steps.Input; +import it.cnr.isti.workflow.manager.ios.IODescriptor; +import it.cnr.isti.workflow.manager.ios.IOType; + +/** + * The effective value of a configurable-as-input field. Shared by the LLM descriptor and the two + * MCP blocks, which each used to answer this differently - the MCP copies returned the configured + * value raw, so a placeholder written there reached the MCP service verbatim. + */ +class ConfigurableInputBindingTest { + + private static Input input(String name, Object value) { + return Input.detached( + IODescriptor.input(name, IOType.TEXT, false, List.of(new IOCapability(IOCapabilityType.TEXT, false))), + value); + } + + @Test + void takesTheValueFromThePortWhenOneCarriesIt() { + assertEquals("from-port", ConfigurableInputBinding.resolve( + "model", "", List.of(input("model", "from-port")), Map.of())); + } + + @Test + void resolvesTheConfiguredValueAsATemplate() { + // The only way a global input can decide such a field: globals reach a block through + // template interpolation, never by feeding a port. + assertEquals("gemini-2.5-pro", ConfigurableInputBinding.resolve( + "model", "${{global.modelName}}", List.of(), Map.of("global.modelName", "gemini-2.5-pro"))); + } + + @Test + void resolvesAnOrdinaryInputNameToo() { + assertEquals("llama3.2:3b", ConfigurableInputBinding.resolve( + "model", "${{chosen}}", List.of(input("chosen", "llama3.2:3b")), Map.of())); + } + + @Test + void leavesAConfiguredLiteralAlone() { + assertEquals("qwen3:8b", ConfigurableInputBinding.resolve( + "model", "qwen3:8b", List.of(), Map.of("global.modelName", "gemini-2.5-pro"))); + } + + @Test + void treatsAnEmptyPortAsNothingSaidRatherThanAsAnErasure() { + // An unfilled port must not blank out a configured value, which would turn a working node + // into one that calls nothing at all. + assertEquals("qwen3:8b", ConfigurableInputBinding.resolve( + "model", "qwen3:8b", List.of(input("model", " ")), Map.of())); + assertEquals("qwen3:8b", ConfigurableInputBinding.resolve( + "model", "qwen3:8b", List.of(input("model", null)), Map.of())); + } + + @Test + void hasNothingToReturnWhenNeitherSideSaysAnything() { + assertNull(ConfigurableInputBinding.resolve("model", null, List.of(), Map.of())); + assertEquals("", ConfigurableInputBinding.resolve("model", "", List.of(), Map.of())); + } + + @Test + void answersTheSameFromAnInputValueMap() { + // Conditional and Switch reach the field from inside a helper that only has the map. + assertEquals("from-port", ConfigurableInputBinding.resolve( + "model", "", Map.of("model", "from-port"), Map.of())); + assertEquals("gemini-2.5-pro", ConfigurableInputBinding.resolve( + "model", "${{global.modelName}}", Map.of(), Map.of("global.modelName", "gemini-2.5-pro"))); + assertEquals("qwen3:8b", ConfigurableInputBinding.resolve( + "model", "qwen3:8b", (Map) null, Map.of())); + } +}