Require MCP completion evidence for LLM blocks
This commit is contained in:
parent
877ad1977e
commit
e655549ef7
|
|
@ -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<LLMBlockType> {
|
|||
@JsonProperty(required = false)
|
||||
List<MCPToolServerBinding> 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<String> requiredSuccessfulMcpTools = List.of();
|
||||
|
||||
@Builder
|
||||
public LLMBlockConfiguration(@NonNull String name,
|
||||
@JsonProperty(value = "llmDescriptor", required = false) LLMDescriptor llmDescriptor,
|
||||
String prompt,
|
||||
List<SkillBinding> skills,
|
||||
List<MCPToolServerBinding> mcpServers) {
|
||||
List<MCPToolServerBinding> mcpServers,
|
||||
List<String> 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<LLMBlockType> {
|
|||
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<LLMBlockType> {
|
|||
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();
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -90,7 +90,7 @@ public class LLMExecutor implements BlockExecutor<LLMBlockType> {
|
|||
// 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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -63,6 +63,16 @@ public class LLMToolLoop {
|
|||
public String run(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential credential, String prompt,
|
||||
List<MCPToolServerBinding> bindings, Map<String, Object> 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<MCPToolServerBinding> bindings, Map<String, Object> executionVariables,
|
||||
ExecutionEventLogger eventLogger, List<String> 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<String> successfulTools = new HashSet<>();
|
||||
Set<String> failedTools = new HashSet<>();
|
||||
Set<String> 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<String> requiredSuccessfulTools, Set<String> successfulTools,
|
||||
Set<String> failedTools, Set<String> attemptedTools) {
|
||||
if (requiredSuccessfulTools == null || requiredSuccessfulTools.isEmpty()) {
|
||||
return;
|
||||
}
|
||||
List<String> 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));
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue