From e655549ef7a1d0aaa9725358b1aeaa3efc8c0a3b Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Wed, 23 Sep 2026 13:15:39 +0200 Subject: [PATCH] Require MCP completion evidence for LLM blocks --- .../configurations/LLMBlockConfiguration.java | 32 ++++++++++- .../executors/blocks/LLMExecutor.java | 2 +- .../executors/blocks/LLMToolLoop.java | 54 +++++++++++++++++-- .../executors/blocks/LLMToolLoopTest.java | 19 +++++++ 4 files changed, 102 insertions(+), 5 deletions(-) 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 ab12e08..14f3c42 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 @@ -13,6 +13,9 @@ import it.cnr.isti.workflow.manager.blocks.factories.TemplateInputs; import it.cnr.isti.workflow.manager.blocks.types.LLMBlockType; import it.cnr.isti.workflow.manager.configurations.annotations.LongText; import it.cnr.isti.workflow.manager.configurations.annotations.Structural; +import it.cnr.isti.workflow.manager.configurations.annotations.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.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.mcp.MCPToolServerBinding; @@ -63,17 +66,32 @@ 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) + @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(); + @Builder public LLMBlockConfiguration(@NonNull String name, @JsonProperty(value = "llmDescriptor", required = false) LLMDescriptor llmDescriptor, String prompt, List skills, - List mcpServers) { + List mcpServers, + List requiredSuccessfulMcpTools) { super(name); this.llmDescriptor = llmDescriptor; 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); } @Override @@ -86,6 +104,7 @@ public class LLMBlockConfiguration extends BlockConfiguration { configuration.name = LLMBlockType.TYPE; configuration.skills = List.of(); configuration.mcpServers = List.of(); + configuration.requiredSuccessfulMcpTools = List.of(); return configuration; } @@ -132,4 +151,15 @@ 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(); + } + } 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 515c477..6d0dfb4 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 @@ -90,7 +90,7 @@ public class LLMExecutor implements BlockExecutor { // loop and its events only exist for a node that was given tools. if (config.getMcpServers() != null && !config.getMcpServers().isEmpty()) { String response = llmToolLoop.run(llmProvider, llmDescriptor, credential, prompt, - config.getMcpServers(), executionVariables, eventLogger); + config.getMcpServers(), executionVariables, eventLogger, config.getRequiredSuccessfulMcpTools()); return Map.of(LLMBlockFactory.OUTPUT_NAME, response); } 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 a7f20fd..8b88e3e 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 @@ -63,6 +63,16 @@ 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) { 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. @@ -71,6 +81,9 @@ public class LLMToolLoop { } Instant deadline = Instant.now().plus(maxDuration); + Set successfulTools = new HashSet<>(); + Set failedTools = new HashSet<>(); + Set attemptedTools = new HashSet<>(); try (MCPToolbox toolbox = MCPToolbox.open(bindings, executionVariables, sessionFactory)) { logEvent(eventLogger, ExecutionEventType.MCP_SESSION_OPENED, "Opened MCP sessions for " + String.join(", ", toolbox.serverIds()), @@ -109,6 +122,7 @@ public class LLMToolLoop { if (looksLikeAWrittenToolCall(turn.content())) { throw new IllegalStateException(describeWrittenToolCall(descriptor.model())); } + assertCompletionEvidence(requiredSuccessfulTools, successfulTools, failedTools, attemptedTools); return turn.content(); } @@ -118,7 +132,14 @@ public class LLMToolLoop { int iterationStart = messages.size(); messages.add(ChatMessage.assistantToolCalls(turn.content(), turn.toolCalls())); for (ToolCall toolCall : turn.toolCalls()) { - messages.add(runTool(toolbox, toolCall, iteration, eventLogger)); + 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()); + } } pruneOlderToolResultsIfOverBudget(messages, iterationStart, prunedIndices, toolsOverheadChars, @@ -133,7 +154,7 @@ public class LLMToolLoop { + " (raise app.llm.tools.max-iterations if the task genuinely needs more)"); } - private ChatMessage runTool(MCPToolbox toolbox, ToolCall toolCall, int iteration, + private ToolRun runTool(MCPToolbox toolbox, ToolCall toolCall, int iteration, ExecutionEventLogger eventLogger) { // A turn can ask for several tools, and a cancellation arriving between two of them should // stop here rather than run the rest of the batch first. @@ -166,7 +187,34 @@ public class LLMToolLoop { logger.debug("MCP tool {} on {} finished in {}ms (error={})", toolCall.name(), result.serverId(), elapsed, result.error()); - return ChatMessage.toolResult(toolCall.id(), toolCall.name(), result.text()); + return new ToolRun(ChatMessage.toolResult(toolCall.id(), toolCall.name(), result.text()), toolCall.name(), result.error()); + } + + private record ToolRun(ChatMessage message, String name, 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. + */ + 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)") + .toList(); + if (!missing.isEmpty()) { + throw new IllegalStateException("LLM node produced an answer without the required successful MCP evidence: " + + String.join(", ", missing)); + } } /** 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 4c9fccf..3fd4771 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 @@ -198,6 +198,25 @@ class LLMToolLoopTest { } } + @Test + void refusesAnAnswerWhenTheConfiguredVerificationToolNeverSucceeded() 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")); + 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"))); + + assertTrue(failure.getMessage().contains("start_service (not attempted)"), failure.getMessage()); + assertTrue(failure.getMessage().contains("browser_snapshot (not attempted)"), failure.getMessage()); + } finally { + server.stop(0); + } + } + @Test void anArgumentThatLooksLikeJsonReachesTheToolAsAString() throws Exception { // The regression the whole native path exists for: the bridge turned this very value into an