diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameter.java b/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameter.java index fabdea7..06b4660 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameter.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/ModelParameter.java @@ -5,14 +5,21 @@ package it.cnr.isti.workflow.manager.llms; /** - * The sampling knobs a caller can set. Named here as an enum so a provider can declare which ones - * it honours, and the executor can report the rest on the run rather than letting a value the user - * set quietly do nothing. + * The knobs a caller can set on a model call. Named here as an enum so a provider can declare which + * ones it honours, and the executor can report the rest on the run rather than letting a value the + * user set quietly do nothing. */ public enum ModelParameter { TEMPERATURE, TOP_P, TOP_K, MAX_TOKENS, - SEED + SEED, + + /** + * Not a sampling knob like the five above but a capability switch, which is why + * {@code LLMProvider.supportedParameters()} no longer defaults to "all of them": a provider has + * to claim this one deliberately. Only the Ollama protocol has it today. + */ + THINKING } 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 b383a8d..2960d22 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 @@ -20,7 +20,12 @@ import jakarta.validation.constraints.Min; import lombok.Builder; /** - * Optional sampling parameters for a model call. + * Optional parameters for a model call: how to sample from it, and whether to let it reason. + * + *

{@code reasoning} is the odd one out - a capability switch rather than a sampling knob, and + * the only one whose empty state triggers a per-model decision rather than simply sending nothing. + * It lives here anyway for the same reason the rest do: the MCP blocks need it and have no + * descriptor. * *

Every component is nullable and independent: null means "leave it to the provider", so an * unset object - or an object with only one field set - behaves exactly as before this existed. @@ -76,7 +81,24 @@ public record ModelParameters( @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) { + Long seed, + + /** + * Empty means "decide per model": the provider asks the model whether it can reason before + * sending anything, so a reasoning model keeps its trace out of the answer and one that + * cannot - gemma, llama3 - is simply not asked to. That is what every existing flow gets, + * and it is what repairs them: the flag used to be sent unconditionally, which Ollama + * answers with {@code 400 "model does not support thinking"}. + * + *

Set it only to overrule that per-model answer - see {@link ReasoningMode} for what the + * two constants mean and why this is not a boolean. + */ + @UiOrder(60) + @UiLabel("Reasoning") + @UiDescription("Leave empty to use reasoning on the models that support it. OFF skips it; ON requires it, and fails on a model that cannot reason.") + @JsonProperty(required = false) + @DefaultsWhenEmpty + ReasoningMode reasoning) { /** Which knobs this object actually sets. Empty when it asks for nothing. */ @JsonIgnore @@ -87,6 +109,7 @@ public record ModelParameters( if (topK != null) declared.add(ModelParameter.TOP_K); if (maxTokens != null) declared.add(ModelParameter.MAX_TOKENS); if (seed != null) declared.add(ModelParameter.SEED); + if (reasoning != null) declared.add(ModelParameter.THINKING); return declared; } diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/ReasoningMode.java b/src/main/java/it/cnr/isti/workflow/manager/llms/ReasoningMode.java new file mode 100644 index 0000000..c0f8bfa --- /dev/null +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/ReasoningMode.java @@ -0,0 +1,41 @@ +// 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 com.fasterxml.jackson.annotation.JsonCreator; + +/** + * Whether a model is asked to reason, when the answer is not left to the model itself. + * + *

Two constants and a null rather than a boolean, because the useful default is a third state: + * unset means "decide per model", which is what repairs the flows this was added for and what every + * existing one keeps. A boolean cannot carry that here - the editor collapses one to true or false + * the moment its group is opened, which would turn reasoning off for models that have it without + * anyone asking. + */ +public enum ReasoningMode { + + /** + * Send the flag whatever the model is. A demand, not a stronger default: a model that cannot + * reason fails the call rather than quietly answering without it. + */ + ON, + + /** + * Never send it. For a model that reasons but where the tokens and latency are not worth it - + * the one case detection cannot guess, since the capability is there. + */ + OFF; + + /** + * A select nobody chose from posts "" rather than leaving the field out, and an enum cannot be + * coerced from that - see {@code MCPAgentUploadKind.fromString}, which this follows. Blank is + * read as unset, which is exactly the per-model default. + */ + @JsonCreator + public static ReasoningMode fromString(String value) { + return value == null || value.isBlank() ? null : valueOf(value.trim().toUpperCase()); + } +} diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/providers/LLMProvider.java b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/LLMProvider.java index 1191917..3a4133a 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/providers/LLMProvider.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/LLMProvider.java @@ -119,12 +119,17 @@ public interface LLMProvider { } /** - * Which sampling parameters this provider actually applies. Everything, by default: a provider + * Which parameters this provider actually applies. Every sampling knob by default: a provider * that maps none of them still behaves as it always has, and only one that knowingly leaves a * knob out should narrow this. + * + *

{@link ModelParameter#THINKING} is deliberately not in the default set, which is why this + * is no longer {@code allOf}. It is a capability only the Ollama protocol has, and a provider + * inheriting a claim to it would mean the run log stays silent about a reasoning switch that + * never reached anything - the exact failure {@code supportedParameters} exists to prevent. */ default Set supportedParameters() { - return EnumSet.allOf(ModelParameter.class); + return EnumSet.complementOf(EnumSet.of(ModelParameter.THINKING)); } /** diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaProtocolProvider.java b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaProtocolProvider.java index 6004764..55b376d 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaProtocolProvider.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaProtocolProvider.java @@ -5,10 +5,14 @@ package it.cnr.isti.workflow.manager.llms.providers.ollama; import java.util.ArrayList; +import java.util.EnumSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -17,18 +21,22 @@ import org.springframework.web.reactive.function.client.WebClient; import tools.jackson.databind.ObjectMapper; import it.cnr.isti.workflow.manager.llms.ChatMessage; +import it.cnr.isti.workflow.manager.llms.ModelParameter; import it.cnr.isti.workflow.manager.llms.ModelParameters; import it.cnr.isti.workflow.manager.llms.ProviderCredential; +import it.cnr.isti.workflow.manager.llms.ReasoningMode; import it.cnr.isti.workflow.manager.llms.ToolCall; import it.cnr.isti.workflow.manager.llms.ToolChatResult; import it.cnr.isti.workflow.manager.llms.ToolDefinition; import it.cnr.isti.workflow.manager.llms.providers.AbstractHttpLLMProvider; +import it.cnr.isti.workflow.manager.llms.providers.LLMProviderHttpException; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ChatResponse; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ChatResponseMessage; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ToolCallResponse; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.GenerateResponse; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ModelInfo; import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ModelResponse; +import it.cnr.isti.workflow.manager.llms.providers.ollama.response.ShowResponse; import reactor.core.publisher.Mono; /** @@ -48,8 +56,32 @@ import reactor.core.publisher.Mono; public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { private static final Logger log = LoggerFactory.getLogger(OllamaProtocolProvider.class); + + /** What Ollama calls the reasoning capability in the {@code /api/show} capability list. */ + private static final String THINKING_CAPABILITY = "thinking"; + + /** + * The error Ollama answers a think:true it will not accept with, lowercased. Matched on the + * text because the status alone (400) is what a misspelled model returns too, and reacting to + * that one by silently retrying would hide it. + */ + private static final String THINKING_REJECTION = "does not support thinking"; + private final ObjectMapper objectMapper = new ObjectMapper(); + /** + * Whether an endpoint and model accept {@code think:true}, remembered so the question is asked + * once rather than per call. Keyed by both because the same model name on two Ollama instances + * can genuinely differ - one of them may be old enough not to validate the flag at all. + * + *

Unbounded on purpose: an entry is one boolean per model actually called, and the set of + * models a deployment calls is small and does not grow on its own. An instance lives as long as + * the application, so a model re-pulled with different capabilities keeps its old answer until + * restart - accepted, because the reasoning capability is a property of the architecture and + * does not change under the same tag in practice. + */ + private final Map thinkAccepted = new ConcurrentHashMap<>(); + protected OllamaProtocolProvider(WebClient.Builder webClientBuilder) { super(webClientBuilder); } @@ -129,14 +161,11 @@ public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { ModelParameters parameters) { Objects.requireNonNull(model, "model cannot be null"); Objects.requireNonNull(prompt, "prompt cannot be null"); - Map body = buildGenerateBody(model, prompt, jsonResponse, parameters); - Mono request = withErrorHandling(clientFor(baseUrl(credential)).post() - .uri("/generate") - .header("Authorization", "Bearer " + resolveApiKey(credential)) - .contentType(MediaType.APPLICATION_JSON) - .bodyValue(body) - .retrieve()); - return parseGenerateResponse(blockingCall(request, "generate", model)); + String baseUrl = baseUrl(credential); + String apiKey = resolveApiKey(credential); + return parseGenerateResponse(callDecidingOnThinking(baseUrl, apiKey, model, parameters, + think -> post(baseUrl, apiKey, "/generate", model, + buildGenerateBody(model, prompt, jsonResponse, parameters, think)))); } @Override @@ -159,14 +188,159 @@ public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { ProviderCredential credential, ModelParameters parameters) { Objects.requireNonNull(model, "model cannot be null"); Objects.requireNonNull(messages, "messages cannot be null"); - Map body = buildChatBody(model, messages, parameters, tools); - Mono request = withErrorHandling(clientFor(baseUrl(credential)).post() - .uri("/chat") - .header("Authorization", "Bearer " + resolveApiKey(credential)) + String baseUrl = baseUrl(credential); + String apiKey = resolveApiKey(credential); + return callDecidingOnThinking(baseUrl, apiKey, model, parameters, + think -> post(baseUrl, apiKey, "/chat", model, + buildChatBody(model, messages, parameters, think, tools))); + } + + /** The one POST both call paths go through, so the retry below re-sends an identical request. */ + private String post(String baseUrl, String apiKey, String path, String model, Map body) { + Mono request = withErrorHandling(clientFor(baseUrl).post() + .uri(path) + .header("Authorization", "Bearer " + apiKey) .contentType(MediaType.APPLICATION_JSON) .bodyValue(body) .retrieve()); - return blockingCall(request, "chat", model); + return blockingCall(request, path.substring(1), model); + } + + /** + * The only provider family with a reasoning switch, so the only one that may claim it - see + * {@code LLMProvider.supportedParameters()} for why the default leaves it out. + */ + @Override + public Set supportedParameters() { + return EnumSet.allOf(ModelParameter.class); + } + + /** One attempt at a call, with the reasoning flag either in the body or left out of it. */ + @FunctionalInterface + private interface ThinkableCall { + String run(boolean think); + } + + /** + * What to do with the reasoning flag on one call, and whether being wrong about it is + * recoverable. + * + *

The distinction that matters is between {@link #AUTO} and {@link #DEMANDED}: both send the + * flag, but only the first may quietly stop sending it. Asking for reasoning explicitly and + * getting an answer produced without it is the failure mode that setting it explicitly is meant + * to rule out. + */ + private enum ThinkDecision { + /** The model cannot reason, or the flow asked for it not to. */ + OFF(false, false), + /** Nobody said either way and the model can reason: send it, and recover if we were wrong. */ + AUTO(true, true), + /** The flow asked for reasoning: a model that cannot must fail, not silently do without. */ + DEMANDED(true, false); + + private final boolean send; + private final boolean recoverable; + + ThinkDecision(boolean send, boolean recoverable) { + this.send = send; + this.recoverable = recoverable; + } + } + + /** + * Runs a call with the reasoning flag resolved for this model, and - when the resolution was a + * guess rather than an answer - repeats it without the flag if the server rejects it. + * + *

The retry is the part that makes this work against any Ollama, including one too old to + * report capabilities and one behind a proxy that will not answer {@code /api/show}. It costs + * one rejected request per model per process, and nothing after that: the rejection is what + * teaches the cache. Nothing else is retried here - the test below matches the server's own + * wording, not the 400, because a misspelled model name is a 400 too and must stay one. + */ + private String callDecidingOnThinking(String baseUrl, String apiKey, String model, + ModelParameters parameters, ThinkableCall call) { + ThinkDecision decision = resolveThinking(baseUrl, apiKey, model, parameters); + try { + return call.run(decision.send); + } catch (RuntimeException error) { + if (!decision.recoverable || !isThinkingRejection(error)) { + throw error; + } + log.info("Model {} on {} does not support thinking: retrying without it, and not asking again", model, + baseUrl); + thinkAccepted.put(cacheKey(baseUrl, model), Boolean.FALSE); + return call.run(false); + } + } + + /** Empty means "decide per model"; a value set on the flow is obeyed as written. */ + private ThinkDecision resolveThinking(String baseUrl, String apiKey, String model, ModelParameters parameters) { + ReasoningMode requested = parameters == null ? null : parameters.reasoning(); + if (requested == ReasoningMode.OFF) { + return ThinkDecision.OFF; + } + if (requested == ReasoningMode.ON) { + return ThinkDecision.DEMANDED; + } + return thinkAccepted.computeIfAbsent(cacheKey(baseUrl, model), key -> probeThinking(baseUrl, apiKey, model)) + ? ThinkDecision.AUTO + : ThinkDecision.OFF; + } + + /** + * Asks the server what this model can do, rather than guessing from its name. + * + *

Optimistic when it cannot tell, in both of the ways it can fail to: a server old enough to + * have no {@code capabilities} field is also old enough not to validate {@code think}, so + * sending it there is exactly what this provider has always done; and a {@code /api/show} that + * errors outright says nothing about the model, so the call itself is left to find out - which + * the retry above then handles. Never throws: a diagnostic endpoint must not be able to fail a + * run that would otherwise have worked. + */ + private boolean probeThinking(String baseUrl, String apiKey, String model) { + try { + Mono request = withErrorHandling(clientFor(baseUrl).post() + .uri("/show") + .header("Authorization", "Bearer " + apiKey) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue(Map.of("model", model)) + .retrieve()); + String body = request.block(); + if (body == null || body.isBlank()) { + return true; + } + List capabilities = objectMapper.readValue(body, ShowResponse.class).getCapabilities(); + if (capabilities == null) { + return true; + } + boolean canThink = capabilities.stream().anyMatch(THINKING_CAPABILITY::equalsIgnoreCase); + log.debug("Model {} on {} reports capabilities {}, thinking {}", model, baseUrl, capabilities, + canThink ? "enabled" : "not requested"); + return canThink; + } catch (RuntimeException probeFailure) { + log.debug("Could not read the capabilities of {} from {} ({}): leaving the reasoning flag on and letting" + + " the call itself say otherwise", model, baseUrl, probeFailure.toString()); + return true; + } + } + + /** + * Matched on the server's wording rather than the status, and walked down the cause chain + * because the timeout and retry operators wrap what they propagate. + */ + private static boolean isThinkingRejection(Throwable error) { + for (Throwable cause = error; cause != null && cause != cause.getCause(); cause = cause.getCause()) { + if (cause instanceof LLMProviderHttpException http + && http.responseBody().toLowerCase(Locale.ROOT).contains(THINKING_REJECTION)) { + return true; + } + } + return false; + } + + /** Both halves, because the same model name on two instances is not the same model. */ + private static String cacheKey(String baseUrl, String model) { + return baseUrl + '\u0000' + model; } /** @@ -206,7 +380,8 @@ public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { } /** Package-private so the request body can be asserted without a server. The defaults it carries are load-bearing. */ - Map buildGenerateBody(String model, String prompt, boolean jsonResponse, ModelParameters parameters) { + Map buildGenerateBody(String model, String prompt, boolean jsonResponse, ModelParameters parameters, + boolean think) { Map bodyMap = new LinkedHashMap<>(); bodyMap.put("model", model); bodyMap.put("prompt", prompt); @@ -216,8 +391,14 @@ public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { // itself. Asking for it explicitly makes Ollama return it separately as "thinking" instead, // which parseGenerateResponse below never reads - so "response" stays just the final // answer, with no loss of reasoning quality (unlike think:false, which would ask the model - // to skip reasoning altogether). Ignored by models/versions that don't support it. - bodyMap.put("think", true); + // to skip reasoning altogether). + // + // Omitted rather than sent as false when this model cannot reason: Ollama answers a + // think:true it does not accept with a 400, and only the flag's absence is what every + // version of it treats as "no opinion". The caller decides - see resolveThinking. + if (think) { + bodyMap.put("think", true); + } if (jsonResponse) { bodyMap.put("format", "json"); // These two have been forced on the JSON path since before parameters existed, and the @@ -239,25 +420,28 @@ public abstract class OllamaProtocolProvider extends AbstractHttpLLMProvider { } /** Mutable, unlike the Map.of it replaces: with nothing set the body is the one it always sent. */ - Map buildChatBody(String model, List messages, ModelParameters parameters) { - return buildChatBody(model, messages, parameters, List.of()); + Map buildChatBody(String model, List messages, ModelParameters parameters, + boolean think) { + return buildChatBody(model, messages, parameters, think, List.of()); } /** * The same body, plus the tools the model may call. * - *

With an empty tool list this is byte for byte the body the three-argument overload has - * always sent, which is what keeps the text path - every existing caller - unchanged. + *

With an empty tool list this is byte for byte the body the shorter overload has always + * sent, which is what keeps the text path - every existing caller - unchanged. */ Map buildChatBody(String model, List messages, ModelParameters parameters, - List tools) { + boolean think, List tools) { Map bodyMap = new LinkedHashMap<>(); bodyMap.put("model", model); bodyMap.put("messages", messages.stream().map(OllamaProtocolProvider::toOllamaMessage).toList()); bodyMap.put("stream", false); // See buildGenerateBody: keeps reasoning out of message.content without asking the model to - // reason less. - bodyMap.put("think", true); + // reason less, and is left out entirely for a model that cannot reason. + if (think) { + bodyMap.put("think", true); + } if (tools != null && !tools.isEmpty()) { bodyMap.put("tools", tools.stream() .map(tool -> Map.of("type", "function", "function", Map.of( diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/response/ShowResponse.java b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/response/ShowResponse.java new file mode 100644 index 0000000..71c43be --- /dev/null +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/providers/ollama/response/ShowResponse.java @@ -0,0 +1,28 @@ +// 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.providers.ollama.response; + +import java.util.List; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; + +import lombok.Data; +import lombok.NoArgsConstructor; + +/** + * What {@code /api/show} says about one model. Only {@code capabilities} is read - the response + * also carries the modelfile, the template and the full parameter dump, none of which we want. + * + *

The list names what the model can do: {@code completion}, {@code tools}, {@code thinking}, + * {@code vision}, {@code insert}, {@code embedding}. Null when the server is older than the field, + * which is not the same as an empty list and must not be read as "can do nothing" - see + * {@code OllamaProtocolProvider.probeThinking}. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +@Data +@NoArgsConstructor +public class ShowResponse { + private List capabilities; +} diff --git a/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPAgentService.java b/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPAgentService.java index 005bc1f..e4661dc 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPAgentService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPAgentService.java @@ -38,6 +38,7 @@ import tools.jackson.databind.ObjectMapper; import it.cnr.isti.workflow.manager.commons.CircuitBreaker; import it.cnr.isti.workflow.manager.llms.ModelParameters; +import it.cnr.isti.workflow.manager.llms.ReasoningMode; import it.cnr.isti.workflow.manager.commons.Throwables; import reactor.core.publisher.Mono; @@ -214,7 +215,16 @@ public class MCPAgentService { // result (see InternalOllamaLLMProvider.buildGenerateBody), so a block's output isn't the // model's raw chain-of-thought. Whether the bridge forwards this to its own Ollama call is // outside this codebase, same as the options below - if it doesn't, this has no effect. - llmProvider.put("think", true); + // + // Unlike the direct Ollama path this cannot ask the model what it can do: the bridge is not + // Ollama and exposes no capability endpoint, so the flag is what the flow says it is and + // "empty" keeps the behaviour this has always had. A flow pointed at a model that cannot + // reason turns it off here by hand - which is why the setting exists as well as the + // detection. Sent only when on, because the flag's absence is the only thing every version + // reads as "no opinion". + if ((parameters == null ? null : parameters.reasoning()) != ReasoningMode.OFF) { + llmProvider.put("think", true); + } if (!ModelParameters.isEmpty(parameters)) { Map options = new LinkedHashMap<>(); if (parameters.temperature() != null) options.put("temperature", parameters.temperature()); @@ -222,7 +232,11 @@ public class MCPAgentService { if (parameters.topK() != null) options.put("top_k", parameters.topK()); if (parameters.maxTokens() != null) options.put("num_predict", parameters.maxTokens()); if (parameters.seed() != null) options.put("seed", parameters.seed()); - llmProvider.put("options", options); + // "think" alone declares parameters without declaring any option: an empty map here + // would be a shape the bridge has never been sent. + if (!options.isEmpty()) { + llmProvider.put("options", options); + } } request.put("llm_provider", llmProvider); if (mcpServers != null && !mcpServers.isEmpty()) { diff --git a/src/test/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoopTest.java b/src/test/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoopTest.java index ced9c3b..8b668f5 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoopTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoopTest.java @@ -164,7 +164,7 @@ class LLMToolLoopTest { private static LLMDescriptor descriptorWithMaxTokens(int maxTokens) { return new LLMDescriptor("Scripted", "test-model", - new it.cnr.isti.workflow.manager.llms.ModelParameters(null, null, null, maxTokens, null)); + new it.cnr.isti.workflow.manager.llms.ModelParameters(null, null, null, maxTokens, null, null)); } private static ObjectNode argumentsWithJsonLookingContent() { 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 index 5fb9e8b..8a04cf2 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/LLMProviderCatalogServiceTest.java @@ -64,7 +64,7 @@ class LLMProviderCatalogServiceTest { 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"), + assertEquals(List.of("MAX_TOKENS", "SEED", "TEMPERATURE", "THINKING", "TOP_K", "TOP_P"), byName.get("Everything").supportedParameters(), "sorted, so the payload is stable"); assertEquals(List.of("TEMPERATURE", "TOP_K"), byName.get("NoSeed").supportedParameters()); assertTrue(byName.get("Everything").supportsTools()); diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/ModelParametersTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/ModelParametersTest.java index fd0b28e..fba8c46 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/llms/ModelParametersTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/ModelParametersTest.java @@ -6,6 +6,7 @@ package it.cnr.isti.workflow.manager.llms; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Set; @@ -43,14 +44,38 @@ class ModelParametersTest { } @Test - void serialisesOnlyTheFiveParametersAndNothingElse() throws Exception { + void anEmptyReasoningSelectionMeansPerModelRatherThanARejectedUpdate() throws Exception { + // A select nobody chose from posts "" instead of leaving the field out, and an enum cannot + // be coerced from that: the whole block update would come back as a bad request naming + // "reasoning". Blank has to land on the per-model default, which is also the safe one. + ModelParameters parameters = new ObjectMapper() + .readValue("{\"reasoning\":\"\"}", ModelParameters.class); + + assertNull(parameters.reasoning()); + assertTrue(parameters.isEmpty()); + } + + @Test + void reasoningIsAThirdStateAndNotAFlagThatDefaultsToOff() { + // The distinction the editor cannot express with a checkbox, and the reason this is an enum: + // "not chosen" has to survive as its own value, or opening the parameter group would turn + // reasoning off for every model that has it. + assertTrue(ModelParameters.builder().build().isEmpty()); + assertEquals(Set.of(ModelParameter.THINKING), + ModelParameters.builder().reasoning(ReasoningMode.OFF).build().declared()); + assertEquals(Set.of(ModelParameter.THINKING), + ModelParameters.builder().reasoning(ReasoningMode.ON).build().declared()); + } + + @Test + void serialisesOnlyTheDeclaredParametersAndNothingElse() throws Exception { // isEmpty() is an is-prefixed no-arg boolean, which Jackson reads as a property: without // @JsonIgnore an "empty" field appeared in every stored flow and every API payload, in a // shape the published schema does not declare. Caught by looking at a real saved block. String json = new ObjectMapper() .writeValueAsString(ModelParameters.builder().temperature(0.0).seed(42L).build()); - assertEquals(Set.of("temperature", "topP", "topK", "maxTokens", "seed"), + assertEquals(Set.of("temperature", "topP", "topK", "maxTokens", "seed", "reasoning"), new ObjectMapper().readTree(json).propertyNames()); } } diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/InternalOllamaLLMProviderBodyTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/InternalOllamaLLMProviderBodyTest.java index 9b53b29..5a0b4ec 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/InternalOllamaLLMProviderBodyTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/InternalOllamaLLMProviderBodyTest.java @@ -37,13 +37,13 @@ class InternalOllamaLLMProviderBodyTest { @Test void nothingSetLeavesTheTextRequestExactlyAsItWas() { // The text path has never sent options. If it started sending an empty map, or defaults it - // did not send before, every existing flow would quietly change behaviour. "think" is sent - // unconditionally regardless (see buildGenerateBody), so a thinking model's reasoning trace - // is never mixed into "response" - unaffected by models/versions that don't support it. + // did not send before, every existing flow would quietly change behaviour. "think" is what + // the caller resolved for this model (see buildGenerateBody), and with a model that can + // reason that is still true - so a thinking model's trace is never mixed into "response". assertEquals(Map.of("model", "m", "prompt", "p", "stream", false, "think", true), - ollama.buildGenerateBody("m", "p", false, null)); + ollama.buildGenerateBody("m", "p", false, null, true)); assertEquals(Map.of("model", "m", "prompt", "p", "stream", false, "think", true), - ollama.buildGenerateBody("m", "p", false, ModelParameters.builder().build())); + ollama.buildGenerateBody("m", "p", false, ModelParameters.builder().build(), true)); } @Test @@ -51,7 +51,7 @@ class InternalOllamaLLMProviderBodyTest { // temperature 0.1 and num_predict 4096 have been forced on the JSON path since before // parameters existed, and the flow assistant's output depends on them. This test exists to // fail if anyone "tidies them away" while making the values configurable. - Map body = ollama.buildGenerateBody("m", "p", true, null); + Map body = ollama.buildGenerateBody("m", "p", true, null, true); assertEquals("json", body.get("format")); assertEquals(Map.of("temperature", 0.1, "num_predict", 4096), body.get("options")); @@ -60,7 +60,7 @@ class InternalOllamaLLMProviderBodyTest { @Test void aSetValueOverridesOnlyItsOwnDefault() { Map body = ollama.buildGenerateBody("m", "p", true, - ModelParameters.builder().temperature(0.9).build()); + ModelParameters.builder().temperature(0.9).build(), true); @SuppressWarnings("unchecked") Map options = (Map) body.get("options"); @@ -72,7 +72,7 @@ class InternalOllamaLLMProviderBodyTest { @Test void mapsEveryParameterToOllamasOwnNames() { Map body = ollama.buildGenerateBody("m", "p", false, ModelParameters.builder() - .temperature(0.2).topP(0.8).topK(40).maxTokens(512).seed(7L).build()); + .temperature(0.2).topP(0.8).topK(40).maxTokens(512).seed(7L).build(), true); assertEquals(Map.of("temperature", 0.2, "top_p", 0.8, "top_k", 40, "num_predict", 512, "seed", 7L), body.get("options")); @@ -82,7 +82,7 @@ class InternalOllamaLLMProviderBodyTest { void sendsOnlyTheParametersThatWereSet() { // A partially filled object must not fill the gaps with invented values. Map body = ollama.buildGenerateBody("m", "p", false, - ModelParameters.builder().seed(3L).build()); + ModelParameters.builder().seed(3L).build(), true); assertEquals(Map.of("seed", 3L), body.get("options")); } @@ -91,7 +91,7 @@ class InternalOllamaLLMProviderBodyTest { void theChatBodyIsUnchangedWithNothingSet() { List messages = List.of(new ChatMessage(ChatMessage.Role.USER, "hi")); - Map body = ollama.buildChatBody("m", messages, null); + Map body = ollama.buildChatBody("m", messages, null, true); assertEquals(4, body.size()); assertEquals(Boolean.TRUE, body.get("think")); @@ -99,11 +99,31 @@ class InternalOllamaLLMProviderBodyTest { assertEquals(List.of(Map.of("role", "user", "content", "hi")), body.get("messages")); } + @Test + void aModelThatCannotReasonGetsNoThinkKeyAtAll() { + // Not think:false. Ollama answers think:true on a model without the capability with a 400, + // and the key's absence is the only state every version of it reads as "no opinion" - so + // the off state has to be an omission, not a false. + Map body = ollama.buildGenerateBody("m", "p", false, null, false); + + assertEquals(Map.of("model", "m", "prompt", "p", "stream", false), body); + assertFalse(body.containsKey("think")); + } + + @Test + void theChatBodyAlsoLeavesThinkOutForAModelThatCannotReason() { + Map body = ollama.buildChatBody("m", + List.of(new ChatMessage(ChatMessage.Role.USER, "hi")), null, false); + + assertFalse(body.containsKey("think")); + assertEquals(3, body.size()); + } + @Test void theChatBodyCarriesTheParametersWhenThereAreSome() { Map body = ollama.buildChatBody("m", List.of(new ChatMessage(ChatMessage.Role.USER, "hi")), - ModelParameters.builder().temperature(0.0).build()); + ModelParameters.builder().temperature(0.0).build(), true); assertEquals(Map.of("temperature", 0.0), body.get("options")); } @@ -111,12 +131,12 @@ class InternalOllamaLLMProviderBodyTest { @Test void anEmptyToolListLeavesTheChatBodyExactlyAsItWas() { // The overload with tools is what every chat call now goes through, so with none declared it - // has to produce the body the three-argument one always did - otherwise adding tool support - // would change every existing flow that never asked for a tool. + // has to produce the body the shorter one always did - otherwise adding tool support would + // change every existing flow that never asked for a tool. List messages = List.of(new ChatMessage(ChatMessage.Role.USER, "hi")); - assertEquals(ollama.buildChatBody("m", messages, null), - ollama.buildChatBody("m", messages, null, List.of())); + assertEquals(ollama.buildChatBody("m", messages, null, true), + ollama.buildChatBody("m", messages, null, true, List.of())); } @Test @@ -125,7 +145,7 @@ class InternalOllamaLLMProviderBodyTest { schema.put("type", "object"); Map body = ollama.buildChatBody("m", - List.of(new ChatMessage(ChatMessage.Role.USER, "hi")), null, + List.of(new ChatMessage(ChatMessage.Role.USER, "hi")), null, true, List.of(new ToolDefinition("write_file", "writes a file", schema))); assertEquals(List.of(Map.of("type", "function", "function", Map.of( @@ -140,7 +160,7 @@ class InternalOllamaLLMProviderBodyTest { Map body = ollama.buildChatBody("m", List.of(ChatMessage.assistantToolCalls("on it", List.of(ToolCall.of(0, "write_file", arguments)))), - null, List.of()); + null, true, List.of()); assertEquals(List.of(Map.of( "role", "assistant", @@ -154,7 +174,7 @@ class InternalOllamaLLMProviderBodyTest { // Which one Ollama reads depends on its version, and picking wrong fails silently: the model // just sees a result it cannot attribute to its call. Map body = ollama.buildChatBody("m", - List.of(ChatMessage.toolResult("call_0", "write_file", "ok")), null, List.of()); + List.of(ChatMessage.toolResult("call_0", "write_file", "ok")), null, true, List.of()); assertEquals(List.of(Map.of( "role", "tool", diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaThinkingSupportTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaThinkingSupportTest.java new file mode 100644 index 0000000..65eceed --- /dev/null +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/providers/ollama/OllamaThinkingSupportTest.java @@ -0,0 +1,222 @@ +// 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.providers.ollama; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.web.reactive.function.client.WebClient; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpServer; + +import it.cnr.isti.workflow.manager.llms.ModelParameters; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; +import it.cnr.isti.workflow.manager.llms.ReasoningMode; +import it.cnr.isti.workflow.manager.llms.providers.LLMProviderHttpException; + +/** + * Whether the reasoning flag is sent, against a stand-in Ollama. + * + *

The behaviour under test is why this needs a server at all: it is a conversation, not a body. + * Ollama rejects {@code think:true} on a model that cannot reason - gemma, llama3 - with a 400, so + * a flag that was previously sent unconditionally broke every flow pointed at one of them. The + * provider now asks {@code /api/show} first, and treats a rejection as the answer when asking was + * not possible. + */ +class OllamaThinkingSupportTest { + + private HttpServer ollama; + private final List generateBodies = new CopyOnWriteArrayList<>(); + private final List shownModels = new CopyOnWriteArrayList<>(); + + /** Set per test before the first call, so each can stage a different server. */ + private volatile List capabilities; + private volatile boolean showFails; + + @BeforeEach + void startOllama() throws IOException { + capabilities = List.of("completion", "thinking"); + showFails = false; + ollama = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + ollama.createContext("/api/show", exchange -> { + shownModels.add(body(exchange)); + if (showFails) { + writeJson(exchange, 500, "{\"error\":\"nope\"}"); + return; + } + writeJson(exchange, 200, capabilities == null + ? "{\"model_info\":{}}" + : "{\"capabilities\":[" + capabilities.stream().map(c -> '"' + c + '"').reduce((a, b) -> a + "," + b) + .orElse("") + "]}"); + }); + ollama.createContext("/api/generate", exchange -> { + String requestBody = body(exchange); + generateBodies.add(requestBody); + // Copied from a real Ollama, quoted model name and all, rather than paraphrased: the + // provider matches on the wording, so this string is the contract. Keying off the bare + // 400 instead would be caught by aRejectionThatIsNotAboutThinkingIsLeftAlone below. + if (requestBody.contains("\"think\"")) { + writeJson(exchange, 400, "{\"error\":\"\\\"gemma:7b\\\" does not support thinking\"}"); + return; + } + writeJson(exchange, 200, "{\"response\":\"answered\"}"); + }); + ollama.start(); + } + + @AfterEach + void stopOllama() { + ollama.stop(0); + } + + private InternalOllamaLLMProvider provider() { + return new InternalOllamaLLMProvider(WebClient.builder(), "test-key", + "http://127.0.0.1:" + ollama.getAddress().getPort() + "/api"); + } + + @Test + void aModelWithoutTheThinkingCapabilityIsNeverAskedToReason() { + capabilities = List.of("completion", "tools"); + + assertEquals("answered", provider().generate("gemma:7b", "hi")); + + assertEquals(1, generateBodies.size()); + assertFalse(generateBodies.get(0).contains("think"), + "a model that cannot reason must not be sent the flag at all"); + } + + @Test + void aModelThatCanReasonStillGetsTheFlag() { + capabilities = List.of("completion", "tools", "thinking"); + + assertEquals("answered", provider().generate("qwen3:8b", "hi")); + + assertTrue(generateBodies.get(0).contains("\"think\":true"), + "a model that reports the capability must still be asked to reason"); + } + + @Test + void aServerThatCannotBeAskedIsRecoveredFromByTheRejectionItself() { + // The case /api/show cannot cover: an Ollama behind a proxy, or one too old to report + // capabilities. The first call is spent finding out, and the answer still comes back. + showFails = true; + + assertEquals("answered", provider().generate("gemma:7b", "hi")); + + assertEquals(2, generateBodies.size(), "one rejected attempt, then the same call without the flag"); + assertTrue(generateBodies.get(0).contains("\"think\":true")); + assertFalse(generateBodies.get(1).contains("think")); + } + + @Test + void aServerTooOldToReportCapabilitiesIsTreatedTheSameWay() { + // No "capabilities" field at all is not an empty capability list: such a version does not + // validate think either, so the flag is sent and the rejection - if any - teaches us. + capabilities = null; + + assertEquals("answered", provider().generate("gemma:7b", "hi")); + + assertEquals(2, generateBodies.size()); + } + + @Test + void theRejectionIsRememberedSoOnlyOneCallIsEverWasted() { + showFails = true; + InternalOllamaLLMProvider provider = provider(); + + assertEquals("answered", provider.generate("gemma:7b", "hi")); + assertEquals("answered", provider.generate("gemma:7b", "again")); + assertEquals("answered", provider.generate("gemma:7b", "and again")); + + assertEquals(4, generateBodies.size(), "the first call pays for the discovery; the rest do not"); + assertTrue(generateBodies.get(0).contains("\"think\":true")); + assertFalse(generateBodies.get(1).contains("think")); + assertFalse(generateBodies.get(2).contains("think")); + assertFalse(generateBodies.get(3).contains("think")); + } + + @Test + void theCapabilityIsAskedForOncePerModelRatherThanPerCall() { + capabilities = List.of("completion"); + InternalOllamaLLMProvider provider = provider(); + + provider.generate("gemma:7b", "hi"); + provider.generate("gemma:7b", "again"); + + assertEquals(1, shownModels.size()); + assertTrue(shownModels.get(0).contains("gemma:7b")); + } + + @Test + void reasoningTurnedOffSkipsEvenAskingTheServer() { + // Nothing to discover: the flow has already said no, so the capability call is wasted work. + capabilities = List.of("completion", "thinking"); + + assertEquals("answered", provider().generate("qwen3:8b", "hi", (ProviderCredential) null, + ModelParameters.builder().reasoning(ReasoningMode.OFF).build())); + + assertTrue(shownModels.isEmpty()); + assertFalse(generateBodies.get(0).contains("think")); + } + + @Test + void reasoningDemandedExplicitlyFailsRatherThanQuietlyRunningWithoutIt() { + // The difference between "on" and "unset": asking for reasoning and getting an answer + // produced without it is the outcome setting it explicitly exists to rule out. + capabilities = List.of("completion"); + + LLMProviderHttpException error = assertThrows(LLMProviderHttpException.class, + () -> provider().generate("gemma:7b", "hi", (ProviderCredential) null, + ModelParameters.builder().reasoning(ReasoningMode.ON).build())); + + assertEquals(400, error.statusCode()); + assertEquals(1, generateBodies.size(), "a demand must not be downgraded by the retry"); + assertTrue(shownModels.isEmpty(), "an explicit value needs no detection"); + } + + @Test + void aRejectionThatIsNotAboutThinkingIsLeftAlone() throws IOException { + // A misspelled model name is a 400 too. Retrying that one without the flag would turn a + // clear error into a second identical failure, and hide which of the two it was. + ollama.removeContext("/api/generate"); + ollama.createContext("/api/generate", exchange -> { + generateBodies.add(body(exchange)); + writeJson(exchange, 400, "{\"error\":\"model 'qwen4' not found\"}"); + }); + capabilities = List.of("completion", "thinking"); + + LLMProviderHttpException error = assertThrows(LLMProviderHttpException.class, + () -> provider().generate("qwen4", "hi")); + + assertTrue(error.responseBody().contains("not found")); + assertEquals(1, generateBodies.size(), "only a thinking rejection is retried"); + } + + private static String body(HttpExchange exchange) throws IOException { + return new String(exchange.getRequestBody().readAllBytes(), StandardCharsets.UTF_8); + } + + private static void writeJson(HttpExchange exchange, int status, String body) throws IOException { + byte[] bytes = body.getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().set("Content-Type", "application/json"); + exchange.sendResponseHeaders(status, bytes.length); + try (var output = exchange.getResponseBody()) { + output.write(bytes); + } + } + +} diff --git a/src/test/java/it/cnr/isti/workflow/manager/mcp/MCPAgentServiceTest.java b/src/test/java/it/cnr/isti/workflow/manager/mcp/MCPAgentServiceTest.java index f436f5c..0f6e5d1 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/mcp/MCPAgentServiceTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/mcp/MCPAgentServiceTest.java @@ -26,6 +26,7 @@ import com.sun.net.httpserver.HttpExchange; import com.sun.net.httpserver.HttpServer; import it.cnr.isti.workflow.manager.llms.ModelParameters; +import it.cnr.isti.workflow.manager.llms.ReasoningMode; import org.junit.jupiter.api.io.TempDir; import org.springframework.core.io.DefaultResourceLoader; import org.springframework.web.reactive.function.client.WebClient; @@ -275,9 +276,8 @@ class MCPAgentServiceTest { void openSessionRequestIsUnchangedWithNoParameters() throws Exception { // The bridge is an external service with no contract in this repo. With nothing set in // ModelParameters, "options" stays absent, so a bridge that knows nothing about options is - // unaffected by their existence. "think" is the one exception: it is sent unconditionally - // (not gated by ModelParameters) so a thinking model's reasoning trace never ends up mixed - // into the block's output, the same way InternalOllamaLLMProvider always asks for it too. + // unaffected by their existence. "think" is the one exception: unset means on, so a + // thinking model's reasoning trace never ends up mixed into the block's output. Map request = serviceWithNoServers() .buildOpenSessionRequest("m", List.of(), Map.of(), null); @@ -286,6 +286,27 @@ class MCPAgentServiceTest { assertFalse(request.containsKey("max_steps")); } + @Test + void reasoningTurnedOffLeavesTheFlagOutEntirely() throws Exception { + // This path cannot ask the bridge what the model can do, so turning it off by hand is the + // only remedy for a model that has no reasoning - and the flag has to be absent, not false, + // because absence is what every Ollama version reads as "no opinion". + Map request = serviceWithNoServers().buildOpenSessionRequest("m", List.of(), Map.of(), + ModelParameters.builder().reasoning(ReasoningMode.OFF).build()); + + assertEquals(Map.of("provider", "ollama", "model", "m"), request.get("llm_provider")); + } + + @Test + void reasoningOnItsOwnDoesNotInventAnEmptyOptionsMap() throws Exception { + // "think" declares parameters without declaring any option. An empty "options" would be a + // shape the bridge has never been sent. + Map request = serviceWithNoServers().buildOpenSessionRequest("m", List.of(), Map.of(), + ModelParameters.builder().reasoning(ReasoningMode.ON).build()); + + assertEquals(Map.of("provider", "ollama", "model", "m", "think", true), request.get("llm_provider")); + } + @Test void openSessionRequestCarriesTheParametersUnderTheProvider() throws Exception {