diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java index ac3fa3b..39d6f1c 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java @@ -40,6 +40,7 @@ import com.github.victools.jsonschema.module.jackson.JacksonSchemaModule; 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.DefaultsWhenEmpty; import it.cnr.isti.workflow.manager.configurations.annotations.Structural; import it.cnr.isti.workflow.manager.configurations.annotations.UiOptionalGroup; import it.cnr.isti.workflow.manager.configurations.annotations.DynamicSchema; @@ -101,6 +102,7 @@ public class JsonSchemaProducer { Map, Map> dynamicSchemaMap = collectDynamicSchemaMetadata(type); Map, Map> longTextMap = collectLongTextMetadata(type); Map, Set> acceptsPlaceholderMap = collectAcceptsVariablePlaceholderMetadata(type); + Map, Set> defaultsWhenEmptyMap = collectDefaultsWhenEmptyMetadata(type); Map, Map> structuralMap = collectStructuralMetadata(type); Map, Map> uiOptionalGroupMap = collectUiOptionalGroupMetadata(type); Map, Map> uiEnabledWhenMap = collectUiEnabledWhenMetadata(type); @@ -119,6 +121,7 @@ public class JsonSchemaProducer { applyDynamicSchemaMetadata(root, getMergedMetadata(dynamicSchemaMap, type)); applyLongTextMetadata(root, getMergedMetadata(longTextMap, type)); applyAcceptsVariablePlaceholderMetadata(root, mergedNames(acceptsPlaceholderMap, type)); + applyDefaultsWhenEmptyMetadata(root, mergedNames(defaultsWhenEmptyMap, type)); applyStructuralMetadata(root, getMergedMetadata(structuralMap, type)); applyUiOptionalGroupMetadata(root, getMergedMetadata(uiOptionalGroupMap, type)); applyUiEnabledWhenMetadata(root, getMergedMetadata(uiEnabledWhenMap, type)); @@ -153,6 +156,7 @@ public class JsonSchemaProducer { metadataClasses.addAll(schemaAllowedValuesMap.keySet()); metadataClasses.addAll(configurableAsInputMap.keySet()); metadataClasses.addAll(acceptsPlaceholderMap.keySet()); + metadataClasses.addAll(defaultsWhenEmptyMap.keySet()); for (Entry entry : definitions.properties()) { if (!(entry.getValue() instanceof ObjectNode classSchema)) { continue; @@ -166,6 +170,7 @@ public class JsonSchemaProducer { applyDynamicSchemaMetadata(classSchema, getMergedMetadata(dynamicSchemaMap, matchedClass)); applyLongTextMetadata(classSchema, getMergedMetadata(longTextMap, matchedClass)); applyAcceptsVariablePlaceholderMetadata(classSchema, mergedNames(acceptsPlaceholderMap, matchedClass)); + applyDefaultsWhenEmptyMetadata(classSchema, mergedNames(defaultsWhenEmptyMap, matchedClass)); applyStructuralMetadata(classSchema, getMergedMetadata(structuralMap, matchedClass)); applyUiOptionalGroupMetadata(classSchema, getMergedMetadata(uiOptionalGroupMap, matchedClass)); applyUiEnabledWhenMetadata(classSchema, getMergedMetadata(uiEnabledWhenMap, matchedClass)); @@ -831,6 +836,64 @@ public class JsonSchemaProducer { } } + private Map, Set> collectDefaultsWhenEmptyMetadata(Class rootClass) { + Map, Set> result = new HashMap<>(); + Set> visited = new HashSet<>(); + Queue> queue = new ArrayDeque<>(); + queue.add(rootClass); + + while (!queue.isEmpty()) { + Class current = queue.poll(); + if (current == null || !visited.add(current) || isTerminalType(current)) { + continue; + } + + Set names = new LinkedHashSet<>(); + for (Field field : current.getDeclaredFields()) { + if (field.getAnnotation(DefaultsWhenEmpty.class) != null) { + names.add(field.getName()); + } + enqueueRelatedTypes(queue, field.getGenericType(), field.getType()); + } + + if (current.isRecord()) { + for (RecordComponent component : current.getRecordComponents()) { + if (component.getAnnotation(DefaultsWhenEmpty.class) != null) { + names.add(component.getName()); + } + enqueueRelatedTypes(queue, component.getGenericType(), component.getType()); + } + } + + if (current.getSuperclass() != null) { + queue.add(current.getSuperclass()); + } + + if (!names.isEmpty()) { + result.put(current, names); + } + } + + return result; + } + + private void applyDefaultsWhenEmptyMetadata(ObjectNode classSchema, Set names) { + if (names == null || names.isEmpty()) { + return; + } + JsonNode propsNode = classSchema.get("properties"); + if (!(propsNode instanceof ObjectNode properties)) { + return; + } + + for (String name : names) { + JsonNode propNode = properties.get(name); + if (propNode instanceof ObjectNode propertySchema) { + propertySchema.put("x-ui-defaults-when-empty", true); + } + } + } + private Map, Map> collectStructuralMetadata(Class rootClass) { Map, Map> result = new HashMap<>(); Set> visited = new HashSet<>(); 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 2e22539..b7bf989 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 @@ -18,6 +18,7 @@ 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.DefaultsWhenEmpty; import it.cnr.isti.workflow.manager.configurations.annotations.SchemaAllowedValues; import it.cnr.isti.workflow.manager.configurations.annotations.UiEnabledWhen; import it.cnr.isti.workflow.manager.configurations.annotations.UiOptionalGroup; @@ -160,6 +161,7 @@ public class MCPAgentBlockConfiguration extends BlockConfiguration - ISTI-CNR +// SPDX-License-Identifier: AGPL-3.0-or-later +// Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. + +package it.cnr.isti.workflow.manager.configurations.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks a field whose empty state is a decision rather than an omission, so the editor offers a way + * back to it once the field holds a value. + * + *

Declared per field instead of inferred from the field being optional, which is what the editor + * used to do: "you may leave this blank" and "leaving this blank means something specific" are + * different claims, and only the second is worth a control. Most optional fields are simply blank. + * + *

The model sampling parameters are the case this exists for: empty means the provider picks, a + * value cannot be un-typed back into that state by clearing the box - that reads as an unfinished + * edit - and nobody knows what number to type to get the provider's own choice back. + * + *

Says nothing about what the default is. When a concrete one is known it belongs in JSON + * Schema's own {@code default}, which the editor already shows alongside; here there is often no + * value to name, only an absence to return to. + */ +@Target({ElementType.FIELD, ElementType.RECORD_COMPONENT}) +@Retention(RetentionPolicy.RUNTIME) +public @interface DefaultsWhenEmpty { +} diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogService.java b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogService.java index 4ba8699..3f704fa 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogService.java @@ -24,7 +24,8 @@ public class LLMProviderCatalogService { public List list() { return providers.values().stream() .map(provider -> new LLMProviderMetadata(provider.getName(), provider.requiresAuthorization(), - provider.requiresEndpoint())) + provider.requiresEndpoint(), + provider.supportedParameters().stream().map(Enum::name).sorted().toList())) .filter(provider -> StringUtils.hasText(provider.name())) .distinct() .sorted(java.util.Comparator.comparing(LLMProviderMetadata::name, String.CASE_INSENSITIVE_ORDER)) diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderMetadata.java b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderMetadata.java index c9db54f..3c7c66c 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderMetadata.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMProviderMetadata.java @@ -4,11 +4,22 @@ package it.cnr.isti.workflow.manager.llms; +import java.util.List; + /** * Public, non-sensitive capabilities used by every LLM selection UI. * * @param requiresEndpoint whether choosing this provider means the credential must also carry a * base URL - see {@link it.cnr.isti.workflow.manager.llms.providers.LLMProvider#requiresEndpoint()}. + * @param supportedParameters which sampling knobs this provider actually applies, by + * {@link ModelParameter} name. Every provider is offered the same five, so + * without this the editor lets a value be set where it does nothing and the + * run only says so afterwards, in a warning nobody was waiting for. */ -public record LLMProviderMetadata(String name, boolean requiresCredential, boolean requiresEndpoint) { +public record LLMProviderMetadata(String name, boolean requiresCredential, boolean requiresEndpoint, + List supportedParameters) { + + public LLMProviderMetadata { + supportedParameters = supportedParameters == null ? List.of() : List.copyOf(supportedParameters); + } } diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameters.java b/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameters.java index 9f4085a..b383a8d 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameters.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameters.java @@ -10,6 +10,7 @@ import java.util.Set; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import it.cnr.isti.workflow.manager.configurations.annotations.DefaultsWhenEmpty; import it.cnr.isti.workflow.manager.configurations.annotations.UiDescription; import it.cnr.isti.workflow.manager.configurations.annotations.UiLabel; import it.cnr.isti.workflow.manager.configurations.annotations.UiOrder; @@ -43,6 +44,7 @@ public record ModelParameters( @UiDescription("Higher values make the output more varied. 0 makes it as repeatable as the model allows.") @DecimalMin("0.0") @DecimalMax("1.0") @JsonProperty(required = false) + @DefaultsWhenEmpty Double temperature, @UiOrder(20) @@ -50,6 +52,7 @@ public record ModelParameters( @UiDescription("Nucleus sampling: consider only the most likely tokens adding up to this probability.") @DecimalMin("0.0") @DecimalMax("1.0") @JsonProperty(required = false) + @DefaultsWhenEmpty Double topP, @UiOrder(30) @@ -57,6 +60,7 @@ public record ModelParameters( @UiDescription("Consider only this many candidate tokens at each step.") @Min(1) @JsonProperty(required = false) + @DefaultsWhenEmpty Integer topK, @UiOrder(40) @@ -64,12 +68,14 @@ public record ModelParameters( @UiDescription("Upper bound on the length of the generated answer.") @Min(1) @JsonProperty(required = false) + @DefaultsWhenEmpty Integer maxTokens, @UiOrder(50) @UiLabel("Seed") @UiDescription("Fixes the randomness, so the same inputs give the same answer. Needed to tell a real change from model noise.") @JsonProperty(required = false) + @DefaultsWhenEmpty Long seed) { /** Which knobs this object actually sets. Empty when it asks for nothing. */ 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 470a6d2..816aea0 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 @@ -629,6 +629,38 @@ public class BlocksControllerTest { assertFalse(prompt.has("x-ui-rows")); } + @Test + public void onlyTheSamplingParametersDeclareTheyDefaultWhenEmpty() { + // The editor used to offer "Use default" on every optional field, inferring it from the + // field not being required - which says you may leave it blank, not that blank means + // something. The claim is now made per field, and the sampling parameters are what it is + // for: blank means the provider picks, and no typed number gets that state back. + BlockConfigurationDescriptor descriptor = blocksController + .getConfigurationDescriptorForType(LLMBlockType.TYPE); + JsonNode schema = (JsonNode) descriptor.schema(); + + JsonNode parameters = findDefinition(schema, "ModelParameters"); + assertNotNull(parameters, "ModelParameters should be a schema definition"); + for (String knob : new String[] { "temperature", "topP", "topK", "maxTokens", "seed" }) { + assertTrue(parameters.path("properties").path(knob).path("x-ui-defaults-when-empty").asBoolean(), + knob + " should declare that empty means the provider decides"); + } + + // prompt is optional too, and must no longer claim a default just for being optional. + assertFalse(schema.path("properties").path("prompt").has("x-ui-defaults-when-empty")); + assertFalse(schema.path("properties").path("skills").has("x-ui-defaults-when-empty")); + } + + private JsonNode findDefinition(JsonNode schema, String name) { + for (String container : new String[] { "definitions", "$defs", "sharedDefinitions" }) { + JsonNode found = schema.path(container).path(name); + if (!found.isMissingNode() && found.has("properties")) { + return found; + } + } + return null; + } + @Test public void chatInteractionSchemaDeclaresUniqueInputNames() { BlockConfigurationDescriptor descriptor = blocksController diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java new file mode 100644 index 0000000..fb010fb --- /dev/null +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java @@ -0,0 +1,64 @@ +// SPDX-FileCopyrightText: 2025-2026 Lucio Lelii - ISTI-CNR +// SPDX-License-Identifier: AGPL-3.0-or-later +// Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. + +package it.cnr.isti.workflow.manager.llms; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.EnumSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.junit.jupiter.api.Test; + +import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; + +/** + * What the editor is told about a provider before anyone runs anything. + * + *

The capabilities exist so a choice that cannot work is not offered: every provider is handed + * the same five sampling parameters, and the ones it ignores were only ever reported afterwards, in + * an execution warning. + */ +class LLMProviderCatalogServiceTest { + + private static LLMProvider provider(String name, Set parameters) { + return new LLMProvider() { + @Override + public String getName() { + return name; + } + + @Override + public List getRegisteredModels() { + return List.of(); + } + + @Override + public String generate(String model, String prompt) { + throw new UnsupportedOperationException("not exercised by this test"); + } + + @Override + public Set supportedParameters() { + return parameters; + } + }; + } + + @Test + void reportsWhichKnobsEachProviderActuallyApplies() { + LLMProviderCatalogService catalog = new LLMProviderCatalogService(Map.of( + "a", provider("Everything", EnumSet.allOf(ModelParameter.class)), + "b", provider("NoSeed", EnumSet.of(ModelParameter.TEMPERATURE, ModelParameter.TOP_K)))); + + Map byName = catalog.list().stream() + .collect(java.util.stream.Collectors.toMap(LLMProviderMetadata::name, metadata -> metadata)); + + assertEquals(List.of("MAX_TOKENS", "SEED", "TEMPERATURE", "TOP_K", "TOP_P"), + byName.get("Everything").supportedParameters(), "sorted, so the payload is stable"); + assertEquals(List.of("TEMPERATURE", "TOP_K"), byName.get("NoSeed").supportedParameters()); + } +}