From 7cabf6aa08cdcc4efae1be0f85ce5504c1562059 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Sat, 25 Jul 2026 12:31:07 +0200 Subject: [PATCH] feat: validate assistant flows with FlowExecutionValidator, raise repair budget to 2 The assistant's validate step only ran jakarta bean-validation on FlowCreateRequest, which cascades into FlowDataValidator (the @ValidFlowStructure constraint on FlowData) but never into each block's own specificConfiguration - Block.specificConfiguration has no @Valid annotation. So per-block constraints (BranchRejoinBlockConfiguration's 2-10 input bound, LLMBlockConfiguration's placeholder-name regex, MCP shared-session producer/consumer wiring, container subflow rules via the execution-level checks, ...) were never actually enforced: the assistant could emit a structurally-broken block and still report valid=true. - FlowAssistantService.validate() now also runs FlowExecutionValidator.collectErrors(), merging its errors with the existing bean-validation errors so the repair loop sees (and can fix) what used to slip through silently. - Default max repair attempts raised from 1 to 2 (still capped at the existing max of 3), since a single fix round often isn't enough once more error classes are actually being caught. - Fixed draftDoesNotFailWhenSharedMemoryFlagsAreIncomplete: its flow was already broken (two MCP consumer-only blocks referencing a shared session neither produces) and is now correctly reported invalid instead of a false-positive valid=true. - Added two regression tests locking in the new coverage and the new default repair budget. --- .../assistant/FlowAssistantService.java | 21 +++-- .../controllers/AssistantControllerTest.java | 91 ++++++++++++++++++- 2 files changed, 105 insertions(+), 7 deletions(-) diff --git a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java index c391f69..9dfbd30 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java @@ -43,6 +43,7 @@ import it.cnr.isti.workflow.manager.blocks.factories.BlockFactory; import it.cnr.isti.workflow.manager.flows.model.Connection; import it.cnr.isti.workflow.manager.flows.model.FlowCreateRequest; import it.cnr.isti.workflow.manager.flows.model.FlowData; +import it.cnr.isti.workflow.manager.flows.validation.FlowExecutionValidator; import it.cnr.isti.workflow.manager.flows.validation.ValidationError; import it.cnr.isti.workflow.manager.flows.validation.ValidationErrorCodec; import it.cnr.isti.workflow.manager.ios.IODescriptor; @@ -59,6 +60,7 @@ public class FlowAssistantService { private static final String INTERNAL_PROVIDER_NAME = "InternalOllama"; private static final String SHARED_MEMORY_SESSION_NAME = "sharedMemorySession"; private static final int DEFAULT_PROVIDER_RETRY_ATTEMPTS = 3; + private static final int DEFAULT_MAX_REPAIR_ATTEMPTS = 2; private static final long DEFAULT_RETRY_BASE_DELAY_MILLIS = 120L; private static final long DEFAULT_RETRY_MAX_DELAY_MILLIS = 800L; private static final ObjectMapper LENIENT_ASSISTANT_MAPPER = JsonMapper.builder() @@ -136,6 +138,9 @@ public class FlowAssistantService { @Autowired private Validator validator; + @Autowired + private FlowExecutionValidator flowExecutionValidator; + @Value("${app.assistant.provider-retry-base-delay-ms:" + DEFAULT_RETRY_BASE_DELAY_MILLIS + "}") private long retryBaseDelayMillis; @@ -224,7 +229,7 @@ public class FlowAssistantService { Integer maxRepairAttempts, ProgressListener progressListener) { LLMProvider provider = resolveInternalProvider(); ResolvedAssistantModels phaseModels = resolveAssistantModels(workflowModel, requestedPhaseModels); - int allowedRepairs = maxRepairAttempts == null ? 1 : maxRepairAttempts; + int allowedRepairs = maxRepairAttempts == null ? DEFAULT_MAX_REPAIR_ATTEMPTS : maxRepairAttempts; int repairs = 0; OperationMode mode = initialMode; FlowCreateRequest flowContext = currentFlow; @@ -1625,12 +1630,9 @@ public class FlowAssistantService { } private List validate(FlowCreateRequest flow) { - Set> violations = validator.validate(flow); - if (violations.isEmpty()) { - return List.of(); - } - List errors = new ArrayList<>(); + + Set> violations = validator.validate(flow); for (ConstraintViolation violation : violations) { List decoded = ValidationErrorCodec.decode(violation.getMessage()); if (decoded.isEmpty()) { @@ -1645,6 +1647,13 @@ public class FlowAssistantService { } } } + + // Bean validation alone misses structural/execution issues (dangling connections, + // an unconnected BranchRejoin input, global-input reference mismatches, container + // subflow rules, ...) that only surface at actual execution time otherwise. + if (flow != null && flow.flow() != null) { + errors.addAll(flowExecutionValidator.collectErrors(flow.flow())); + } return errors; } diff --git a/src/test/java/it/cnr/isti/workflow/manager/controllers/AssistantControllerTest.java b/src/test/java/it/cnr/isti/workflow/manager/controllers/AssistantControllerTest.java index f07765c..61a7c8e 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/controllers/AssistantControllerTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/controllers/AssistantControllerTest.java @@ -573,6 +573,12 @@ public class AssistantControllerTest { @Test public void draftDoesNotFailWhenSharedMemoryFlagsAreIncomplete() { + // "Does not fail" means draft() still returns a response instead of throwing, even + // though this flow is genuinely broken (both blocks are consumer-only, so neither + // produces the shared session they reference). FlowExecutionValidator now catches + // that as an execution-level error - previously only bean-validation ran here, which + // doesn't check MCP shared-session producer/consumer wiring, so this used to be + // reported as valid=true despite never being executable. mockSharedMemoryResponsesWithIncompleteFlags(); AssistantFlowResponse response = assistantController.draft( @@ -582,7 +588,8 @@ public class AssistantControllerTest { 1)); assertNotNull(response); - assertTrue(response.valid()); + assertFalse(response.valid()); + assertFalse(response.validationErrors().isEmpty()); assertEquals(2, response.flow().flow().getBlocks().size()); } @@ -1536,6 +1543,88 @@ public class AssistantControllerTest { throw new AssertionError("Assistant call did not complete in time"); } + @Test + public void draftCatchesConfigurationConstraintNotCoveredByStructuralValidation() { + // FlowDataValidator (bean-validation cascade) never validates a block's own + // specificConfiguration constraints; only FlowExecutionValidator does, per-block. + // Before wiring FlowExecutionValidator into the assistant's validate step, an invalid + // placeholder name here would have silently passed as valid=true. + mockBlockConfigWithInvalidPlaceholderThenValidAfter(1); + + AssistantFlowResponse response = assistantController.draft( + new AssistantGenerationRequest( + "create a flow that greets a person by name", + MODEL, + 1)); + + assertNotNull(response); + assertTrue(response.valid()); + assertEquals(1, response.repairAttempts()); + LLMBlockConfiguration configuration = (LLMBlockConfiguration) response.flow().flow().getBlocks().getFirst() + .getSpecificConfiguration(); + assertEquals("Greet ${{name}}", configuration.getPrompt()); + } + + @Test + public void draftDefaultsToTwoRepairAttemptsWhenNotSpecified() { + // Needs two repair rounds to succeed - only possible now that the default repair + // budget is 2 instead of 1. + mockBlockConfigWithInvalidPlaceholderThenValidAfter(2); + + AssistantFlowResponse response = assistantController.draft( + new AssistantGenerationRequest( + "create a flow that greets a person by name", + MODEL, + null)); + + assertNotNull(response); + assertTrue(response.valid()); + assertEquals(2, response.repairAttempts()); + } + + private void mockBlockConfigWithInvalidPlaceholderThenValidAfter(int validAfterAttempt) { + AtomicInteger blockConfigAttempts = new AtomicInteger(); + Answer answer = invocation -> { + String prompt = invocation.getArgument(1, String.class); + if (prompt.contains("TASK: PLAN")) { + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "Planned a single greeting step.", + "plan", java.util.Map.of( + "name", "Greeting flow", + "description", "Greet a person by name.", + "blocks", java.util.List.of( + java.util.Map.of( + "blockId", "b1", + "blockType", "LLMBlock", + "purpose", "Greet the person"))))); + } + if (prompt.contains("TASK: BLOCK_CONFIG")) { + if (blockConfigAttempts.incrementAndGet() <= validAfterAttempt) { + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "Configured the greeting block with an invalid placeholder name.", + "block", java.util.Map.of( + "blockId", "b1", + "name", "Greeter", + "config", java.util.Map.of( + "prompt", "Greet ${{bad name}}")))); + } + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "Configured the greeting block.", + "block", java.util.Map.of( + "blockId", "b1", + "name", "Greeter", + "config", java.util.Map.of( + "prompt", "Greet ${{name}}")))); + } + throw new IllegalStateException("Unexpected assistant prompt:\n" + prompt); + }; + + Mockito.when(internalOllamaLLMProvider.generate(Mockito.anyString(), Mockito.anyString())) + .thenAnswer(answer); + Mockito.when(internalOllamaLLMProvider.generateJson(Mockito.anyString(), Mockito.anyString())) + .thenAnswer(answer); + } + static class TestAssistantResponses { private static final String PROVIDER = "InternalOllama";