From 4eb962ed9bd86ec4d4de0eaecba6f813fc3db2a1 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Fri, 18 Sep 2026 11:52:00 +0200 Subject: [PATCH] Offer "Use default" only where a field declares one The editor derived the control from the field being optional, which put it on nearly every field in every dialog. "You may leave this blank" and "leaving this blank means something specific" are different claims, and only the second is worth a control. The claim is now made per field with @DefaultsWhenEmpty, published as x-ui-defaults-when-empty. It goes on the five sampling parameters - where empty means the provider decides, and no typed number gets that state back - and on the three fields that declare a concrete default, which the editor already names alongside. The value itself still comes from JSON Schema's own `default`: a parameter has no value to name, only an absence to return to, so declaring `default: null` would have said something false to every other reader of the schema. Providers also now report which sampling parameters they actually apply. All five were offered to every provider and the unsupported ones were dropped at run time, reported in a warning on an execution that had already happened - Gemini applies no seed, the OpenAI-protocol providers no top_k. Co-Authored-By: Claude Opus 5 (1M context) --- .../configurations/JsonSchemaProducer.java | 63 ++++++++++++++++++ .../MCPAgentBlockConfiguration.java | 2 + .../MCPAgentChatBlockConfiguration.java | 2 + .../configurations/MCPAgentUploadInput.java | 2 + .../annotations/DefaultsWhenEmpty.java | 31 +++++++++ .../llms/LLMProviderCatalogService.java | 3 +- .../manager/llms/LLMProviderMetadata.java | 13 +++- .../manager/llms/ModelParameters.java | 6 ++ .../controllers/BlocksControllerTest.java | 32 ++++++++++ .../llms/LLMProviderCatalogServiceTest.java | 64 +++++++++++++++++++ 10 files changed, 216 insertions(+), 2 deletions(-) create mode 100644 src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/DefaultsWhenEmpty.java create mode 100644 src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java 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()); + } +}