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 9dfa977..7c75a87 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 @@ -1449,9 +1449,6 @@ public class JsonSchemaProducer { equalsAny.add(value); } } - if (dependency.present()) { - visibleWhen.put("present", true); - } } } diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockConfiguration.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockConfiguration.java index 4dc1888..918bf9b 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockConfiguration.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockConfiguration.java @@ -18,7 +18,6 @@ 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; import it.cnr.isti.workflow.manager.configurations.annotations.UiUniqueItemsBy; -import it.cnr.isti.workflow.manager.configurations.annotations.UiVisibleWhen; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.mcp.MCPToolServerBinding; import it.cnr.isti.workflow.manager.skills.SkillBinding; @@ -72,21 +71,6 @@ public class LLMBlockConfiguration extends BlockConfiguration { @JsonProperty(required = false) List mcpServers = List.of(); - /** - * Evidence a tool-using node must collect before its natural-language answer is accepted. - * This is deliberately opt-in: many ordinary MCP nodes only retrieve information and have no - * browser or service lifecycle to prove. - */ - @Structural - @Size(max = 16) - @UiOrder(70) - // Checked only by the loop MCP servers start, so without one there is nothing it could apply to. - @UiVisibleWhen(field = "mcpServers", present = true) - @UiLabel("Required successful MCP tools") - @UiDescription("Tool names that must each complete successfully before this node can finish. Use this for agents that must prove work, for example start_service, browser_navigate and browser_snapshot.") - @JsonProperty(required = false) - List requiredSuccessfulMcpTools = List.of(); - /** * Fields the answer is split into, each becoming a port of its own. * @@ -125,7 +109,6 @@ public class LLMBlockConfiguration extends BlockConfiguration { String prompt, List skills, List mcpServers, - List requiredSuccessfulMcpTools, List outputs, List uploadInputs) { super(name); @@ -133,7 +116,6 @@ public class LLMBlockConfiguration extends BlockConfiguration { this.prompt = prompt; this.skills = skills == null ? List.of() : List.copyOf(skills); this.mcpServers = mcpServers == null ? List.of() : List.copyOf(mcpServers); - this.requiredSuccessfulMcpTools = requiredSuccessfulMcpTools == null ? List.of() : List.copyOf(requiredSuccessfulMcpTools); this.outputs = outputs == null ? List.of() : List.copyOf(outputs); this.uploadInputs = uploadInputs == null ? List.of() : List.copyOf(uploadInputs); } @@ -148,7 +130,6 @@ public class LLMBlockConfiguration extends BlockConfiguration { configuration.name = LLMBlockType.TYPE; configuration.skills = List.of(); configuration.mcpServers = List.of(); - configuration.requiredSuccessfulMcpTools = List.of(); configuration.outputs = List.of(); configuration.uploadInputs = List.of(); return configuration; @@ -215,17 +196,6 @@ public class LLMBlockConfiguration extends BlockConfiguration { return names.stream().distinct().count() == names.size(); } - @AssertTrue(message = "requiredSuccessfulMcpTools must contain distinct non-empty tool names") - @JsonIgnore - boolean areRequiredSuccessfulMcpToolsValid() { - if (requiredSuccessfulMcpTools == null || requiredSuccessfulMcpTools.isEmpty()) { - return true; - } - return requiredSuccessfulMcpTools.stream() - .allMatch(tool -> tool != null && !tool.isBlank()) - && requiredSuccessfulMcpTools.stream().distinct().count() == requiredSuccessfulMcpTools.size(); - } - @AssertTrue(message = "structured output names must be distinct, and none may be called 'response'") @JsonIgnore boolean areOutputNamesUsable() { diff --git a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java index 5d1001d..2a04df1 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java +++ b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java @@ -26,6 +26,4 @@ public @interface UiVisibleWhen { String equals() default ""; String[] equalsAny() default {}; - /** Shown only once {@link #field()} holds something: a text that is not blank, a list with an item. */ - boolean present() default false; } diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMExecutor.java index 8e88004..9650486 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMExecutor.java @@ -113,7 +113,7 @@ public class LLMExecutor implements BlockExecutor { try { if (config.getMcpServers() != null && !config.getMcpServers().isEmpty()) { response = llmToolLoop.run(llmProvider, llmDescriptor, credential, prompt, attached.attachments(), - config.getMcpServers(), executionVariables, eventLogger, config.getRequiredSuccessfulMcpTools()); + config.getMcpServers(), executionVariables, eventLogger); return StructuredLLMOutputs.split(response, config.getOutputs(), LLMBlockFactory.OUTPUT_NAME); } // Files need a message to ride on; without them this stays the single prompt it always was. diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoop.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoop.java index e967674..7f2065d 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoop.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/LLMToolLoop.java @@ -18,6 +18,7 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; +import it.cnr.isti.workflow.manager.executions.NodeExecutionException; import it.cnr.isti.workflow.manager.executions.ExecutionEventLogger; import it.cnr.isti.workflow.manager.executions.ExecutionEventType; import it.cnr.isti.workflow.manager.llms.ChatAttachment; @@ -64,27 +65,20 @@ public class LLMToolLoop { public String run(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential credential, String prompt, List bindings, Map executionVariables, ExecutionEventLogger eventLogger) { - return run(provider, descriptor, credential, prompt, bindings, executionVariables, eventLogger, List.of()); - } - - /** - * Runs an agent with optional evidence requirements. A tool can fail transiently and be retried, - * so requirements are checked only when the model tries to end the conversation. - */ - public String run(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential credential, String prompt, - List bindings, Map executionVariables, - ExecutionEventLogger eventLogger, List requiredSuccessfulTools) { - return run(provider, descriptor, credential, prompt, List.of(), bindings, executionVariables, eventLogger, - requiredSuccessfulTools); + return run(provider, descriptor, credential, prompt, List.of(), bindings, executionVariables, eventLogger); } /** + * Runs an agent. A server bound as required must have answered at least one call successfully + * before the model's answer is accepted; a call can fail transiently and be retried, so that is + * checked only when the model tries to end the conversation. + * * @param attachments files sent with the opening message - the one message pruning never * touches, so the model can look at them again on every turn */ public String run(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential credential, String prompt, List attachments, List bindings, Map executionVariables, - ExecutionEventLogger eventLogger, List requiredSuccessfulTools) { + ExecutionEventLogger eventLogger) { if (!provider.supportsTools()) { // Checked here and not only when the flow is saved: the provider can arrive from an input // port, and then no save-time check ever saw it. @@ -93,9 +87,8 @@ public class LLMToolLoop { } Instant deadline = Instant.now().plus(maxDuration); - Set successfulTools = new HashSet<>(); - Set failedTools = new HashSet<>(); - Set attemptedTools = new HashSet<>(); + Set serversAnswered = new HashSet<>(); + Set serversFailed = new HashSet<>(); try (MCPToolbox toolbox = MCPToolbox.open(bindings, executionVariables, sessionFactory)) { logEvent(eventLogger, ExecutionEventType.MCP_SESSION_OPENED, "Opened MCP sessions for " + String.join(", ", toolbox.serverIds()), @@ -134,7 +127,7 @@ public class LLMToolLoop { if (looksLikeAWrittenToolCall(turn.content())) { throw new IllegalStateException(describeWrittenToolCall(descriptor.model())); } - assertCompletionEvidence(requiredSuccessfulTools, successfulTools, failedTools, attemptedTools); + assertRequiredServersUsed(bindings, serversAnswered, serversFailed); return turn.content(); } @@ -146,11 +139,9 @@ public class LLMToolLoop { for (ToolCall toolCall : turn.toolCalls()) { ToolRun toolRun = runTool(toolbox, toolCall, iteration, eventLogger); messages.add(toolRun.message()); - attemptedTools.add(toolRun.name()); - if (toolRun.error()) { - failedTools.add(toolRun.name()); - } else { - successfulTools.add(toolRun.name()); + // A made-up tool name has no server: it counts for none of them. + if (toolRun.serverId() != null) { + (toolRun.error() ? serversFailed : serversAnswered).add(toolRun.serverId()); } } @@ -199,36 +190,35 @@ public class LLMToolLoop { logger.debug("MCP tool {} on {} finished in {}ms (error={})", toolCall.name(), result.serverId(), elapsed, result.error()); - return new ToolRun(ChatMessage.toolResult(toolCall.id(), toolCall.name(), result.text()), toolCall.name(), result.error()); + return new ToolRun(ChatMessage.toolResult(toolCall.id(), toolCall.name(), result.text()), toolCall.name(), + result.serverId(), result.error()); } - private record ToolRun(ChatMessage message, String name, boolean error) { + private record ToolRun(ChatMessage message, String name, String serverId, boolean error) { } /** - * A prose answer is not evidence that an agent built or tested anything. A flow opting into - * these requirements gets a clear, actionable failure instead of a false SUCCESS when the - * model gives up after an unsuccessful tool sequence. + * A prose answer is not evidence that an agent built or tested anything. A server bound as + * required that never answered a call means the model finished without doing what the flow + * relies on, and the step says which server, rather than passing on a report nobody checked. */ - private static void assertCompletionEvidence(List requiredSuccessfulTools, Set successfulTools, - Set failedTools, Set attemptedTools) { - if (requiredSuccessfulTools == null || requiredSuccessfulTools.isEmpty()) { - return; - } - List missing = requiredSuccessfulTools.stream() - .filter(required -> !successfulTools.contains(required)) - .map(required -> failedTools.contains(required) - ? required + " (attempted but failed)" - : attemptedTools.contains(required) - ? required + " (attempted without a successful result)" - : required + " (not attempted)") + private static void assertRequiredServersUsed(List bindings, Set serversAnswered, + Set serversFailed) { + List unused = bindings.stream() + .filter(MCPToolServerBinding::required) + .map(MCPToolServerBinding::serverName) + .filter(server -> !serversAnswered.contains(server)) + .map(server -> server + (serversFailed.contains(server) ? " (every call to it failed)" : " (never called)")) .toList(); - if (!missing.isEmpty()) { - throw new IllegalStateException("LLM node produced an answer without the required successful MCP evidence: " - + String.join(", ", missing)); + if (!unused.isEmpty()) { + throw new NodeExecutionException(REQUIRED_SERVER_UNUSED, "The model finished without a successful call to" + + " the MCP servers this node requires: " + String.join(", ", unused) + + ". Its answer is not accepted without them"); } } + static final String REQUIRED_SERVER_UNUSED = "LLM_REQUIRED_MCP_SERVER_UNUSED"; + /** * Below this, a placeholder costs about as much as the result it replaces - pruning it would move * the count without moving the needle, and would only make the reported "reclaimed" figure noise. diff --git a/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPToolServerBinding.java b/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPToolServerBinding.java index 8b1ada4..88fbb4d 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPToolServerBinding.java +++ b/src/main/java/it/cnr/isti/workflow/manager/mcp/MCPToolServerBinding.java @@ -42,8 +42,26 @@ public record MCPToolServerBinding( @NotNull @JsonProperty(required = false) @DynamicSchema(url = "/retriever/MCPServers/definitions/schema", dependsOn = { "serverName" }) - JsonNode configuration) { + JsonNode configuration, + + /** + * Whether the node must have used this server before its answer is believed. An agent can + * report building and testing something without having called a single tool, and the + * report reads the same either way. + */ + @JsonProperty(required = false) + @UiLabel("Must be used") + @UiDescription("The node fails if the model finishes without at least one successful call to this server.") + Boolean required) { /** Kept low on purpose: every bound server is a session opened for the life of the block. */ public static final int MAX_PER_BLOCK = 4; + + public MCPToolServerBinding { + required = required != null && required; + } + + public MCPToolServerBinding(String serverName, JsonNode configuration) { + this(serverName, configuration, false); + } } diff --git a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockSchemaTest.java b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockSchemaTest.java index eae12e7..898a51f 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockSchemaTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/LLMBlockSchemaTest.java @@ -21,16 +21,6 @@ class LLMBlockSchemaTest { @Autowired private JsonSchemaProducer schemaProducer; - @Test - void theRequiredToolsAreOfferedOnlyToANodeThatHasMcpServersToCall() { - JsonNode schema = schemaProducer.generateSchemaNode(LLMBlockConfiguration.class); - - JsonNode visibleWhen = schema.get("properties").get("requiredSuccessfulMcpTools").get("x-ui-visible-when"); - - assertEquals("mcpServers", visibleWhen.get("field").asText()); - assertEquals(true, visibleWhen.get("present").asBoolean()); - } - @Test void theFieldsReadInTheOrderANodeIsSetUp() { // The model; what it is asked and given; what it may use; what it hands back. @@ -40,6 +30,6 @@ class LLMBlockSchemaTest { schema.get("x-ui-property-order").forEach(name -> order.add(name.asText())); assertEquals(java.util.List.of("name", "llmDescriptor", "prompt", "uploadInputs", "skills", "mcpServers", - "requiredSuccessfulMcpTools", "outputs"), order.stream().filter(name -> !name.equals("type")).toList()); + "outputs"), order.stream().filter(name -> !name.equals("type")).toList()); } } 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 7c5e0f0..080791c 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 @@ -29,6 +29,7 @@ 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.executions.NodeExecutionException; import it.cnr.isti.workflow.manager.executions.ExecutionEventLogger; import it.cnr.isti.workflow.manager.executions.ExecutionEventType; import it.cnr.isti.workflow.manager.llms.ChatAttachment; @@ -159,6 +160,10 @@ class LLMToolLoopTest { .build()); } + private List requiredBindings() { + return List.of(new MCPToolServerBinding("coding", JsonNodeFactory.instance.objectNode(), true)); + } + private static LLMDescriptor descriptor() { return new LLMDescriptor("Scripted", "test-model", null); } @@ -209,7 +214,7 @@ class LLMToolLoopTest { ChatAttachment sketch = new ChatAttachment(ChatAttachment.Kind.IMAGE, "image/png", "sketch.png", new byte[] { 1 }); try { loopFor(server, 10, 60).run(provider, descriptor(), null, "build what the sketch shows", List.of(sketch), - bindings(), Map.of(), null, List.of()); + bindings(), Map.of(), null); // The model can look at the sketch again after each tool result, not only on the first turn. for (List conversation : provider.seenConversations) { @@ -221,19 +226,50 @@ class LLMToolLoopTest { } @Test - void refusesAnAnswerWhenTheConfiguredVerificationToolNeverSucceeded() throws Exception { + void refusesAnAnswerWhenARequiredServerNeverAnsweredACall() throws Exception { + HttpServer server = startServer(); + ScriptedProvider provider = new ScriptedProvider(true); + provider.turns.add(ToolChatResult.text("the page is ready")); + try { + NodeExecutionException failure = assertThrows(NodeExecutionException.class, + () -> loopFor(server, 10, 60).run(provider, descriptor(), null, "do it", requiredBindings(), Map.of(), + null)); + + assertEquals(LLMToolLoop.REQUIRED_SERVER_UNUSED, failure.getErrorCode()); + assertTrue(failure.getMessage().contains("coding (never called)"), failure.getMessage()); + } finally { + server.stop(0); + } + } + + @Test + void acceptsTheAnswerOnceARequiredServerHasAnsweredACall() throws Exception { HttpServer server = startServer(); ScriptedProvider provider = new ScriptedProvider(true); provider.turns.add(ToolChatResult.toolCalls("writing it", List.of(ToolCall.of(0, "write_file", argumentsWithJsonLookingContent())))); - provider.turns.add(ToolChatResult.text("the page is ready")); + provider.turns.add(ToolChatResult.text("DONE")); try { - IllegalStateException failure = assertThrows(IllegalStateException.class, - () -> loopFor(server, 10, 60).run(provider, descriptor(), null, "do it", bindings(), Map.of(), - null, List.of("start_service", "browser_snapshot"))); + assertEquals("DONE", loopFor(server, 10, 60).run(provider, descriptor(), null, "do it", requiredBindings(), + Map.of(), null)); + } finally { + server.stop(0); + } + } - assertTrue(failure.getMessage().contains("start_service (not attempted)"), failure.getMessage()); - assertTrue(failure.getMessage().contains("browser_snapshot (not attempted)"), failure.getMessage()); + @Test + void aMadeUpToolNameDoesNotCountAsUsingARequiredServer() throws Exception { + HttpServer server = startServer(); + ScriptedProvider provider = new ScriptedProvider(true); + provider.turns.add(ToolChatResult.toolCalls("", + List.of(ToolCall.of(0, "no_such_tool", JsonNodeFactory.instance.objectNode())))); + provider.turns.add(ToolChatResult.text("DONE")); + try { + NodeExecutionException failure = assertThrows(NodeExecutionException.class, + () -> loopFor(server, 10, 60).run(provider, descriptor(), null, "do it", requiredBindings(), Map.of(), + null)); + + assertTrue(failure.getMessage().contains("coding (never called)"), failure.getMessage()); } finally { server.stop(0); } diff --git a/src/test/java/it/cnr/isti/workflow/manager/flows/AgentBuiltHumanJudgedFlowTest.java b/src/test/java/it/cnr/isti/workflow/manager/flows/AgentBuiltHumanJudgedFlowTest.java index 2179b5a..6f111d3 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/flows/AgentBuiltHumanJudgedFlowTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/flows/AgentBuiltHumanJudgedFlowTest.java @@ -94,9 +94,10 @@ class AgentBuiltHumanJudgedFlowTest { .filter(block -> "build-and-check".equals(block.getName())) .findFirst().orElseThrow().getSpecificConfiguration(); - assertTrue(agent.getRequiredSuccessfulMcpTools().containsAll( - List.of("start_service", "browser_navigate", "browser_snapshot")), - agent.getRequiredSuccessfulMcpTools().toString()); + assertEquals(List.of("dev-server-mcp", "browser-mcp"), agent.getMcpServers().stream() + .filter(it.cnr.isti.workflow.manager.mcp.MCPToolServerBinding::required) + .map(it.cnr.isti.workflow.manager.mcp.MCPToolServerBinding::serverName) + .toList(), "it must have started the service and looked at the page"); } @Test diff --git a/src/test/resources/flows/agent-built-human-judged.json b/src/test/resources/flows/agent-built-human-judged.json index 814a4d3..f53b9ec 100644 --- a/src/test/resources/flows/agent-built-human-judged.json +++ b/src/test/resources/flows/agent-built-human-judged.json @@ -63,18 +63,15 @@ }, { "serverName": "dev-server-mcp", - "configuration": {} + "configuration": {}, + "required": true }, { "serverName": "browser-mcp", - "configuration": {} + "configuration": {}, + "required": true } ], - "requiredSuccessfulMcpTools": [ - "start_service", - "browser_navigate", - "browser_snapshot" - ], "outputs": [ { "name": "previewUrl", @@ -351,4 +348,4 @@ } ] } -} \ No newline at end of file +}