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 fd61179..fe9438a 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 @@ -1134,11 +1134,12 @@ public class FlowAssistantService { * so the PLAN and CONNECTIONS phases can be skipped for this FIX round. */ private boolean isTargetedBlockRepairEligible(FlowCreateRequest currentFlow, List errors) { - // Reusing the plan verbatim (buildReusedPlanForTargetedRepair) only reconstructs the - // block list - it would silently drop any existing containers. Bail out to the full - // repair path whenever containers are present rather than risk that. - if (errors == null || errors.isEmpty() || !hasCurrentFlowBlocks(currentFlow) - || hasCurrentFlowContainers(currentFlow)) { + // buildReusedPlanForTargetedRepair carries existing containers forward as KEEP (untouched), + // so this stays eligible even when the flow has containers - as long as every error is + // still scoped to an existing *top-level block*. An error scoped to a container (or + // anything else) can't be fixed by a KEEP-only container plan, so it still disqualifies + // targeted repair and falls back to a full replan. + if (errors == null || errors.isEmpty() || !hasCurrentFlowBlocks(currentFlow)) { return false; } Set existingBlockIds = currentFlow.flow().getBlocks().stream() @@ -1172,7 +1173,21 @@ public class FlowAssistantService { block.getName(), erroringBlockIds.contains(block.getId()) ? PlanOperation.UPDATE.name() : PlanOperation.KEEP.name())) .toList(); - AssistantFlowPlan plan = new AssistantFlowPlan(currentFlow.name(), currentFlow.description(), blockPlans); + // isTargetedBlockRepairEligible only allows this path when every error is scoped to a + // top-level block, so any existing containers are always untouched here - just carried + // forward as KEEP so they aren't silently dropped from the rebuilt plan. + List containerPlans = hasCurrentFlowContainers(currentFlow) + ? currentFlow.flow().getContainers().stream() + .map(container -> new AssistantContainerPlan( + container.getId(), + container.getType().getName(), + container.getName(), + PlanOperation.KEEP.name(), + List.of())) + .toList() + : List.of(); + AssistantFlowPlan plan = new AssistantFlowPlan(currentFlow.name(), currentFlow.description(), blockPlans, + containerPlans); return new ParsedPlan(plan, "Targeted repair: reconfiguring only the blocks flagged by validation 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 7483473..8fcd45b 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 @@ -49,6 +49,9 @@ import it.cnr.isti.workflow.manager.blocks.types.HTTPServerCallBlockType; import it.cnr.isti.workflow.manager.blocks.types.HumanInteractionBlockType; import it.cnr.isti.workflow.manager.blocks.types.LLMBlockType; import it.cnr.isti.workflow.manager.containers.Container; +import it.cnr.isti.workflow.manager.containers.configurations.GenericContainerConfiguration; +import it.cnr.isti.workflow.manager.containers.factories.GenericContainerFactory; +import it.cnr.isti.workflow.manager.containers.types.GenericContainerType; import it.cnr.isti.workflow.manager.blocks.types.MCPAgentBlockType; import it.cnr.isti.workflow.manager.flows.model.Connection; import it.cnr.isti.workflow.manager.flows.model.FlowCreateRequest; @@ -89,6 +92,9 @@ public class AssistantControllerTest { @MockitoBean private InternalOllamaLLMProvider internalOllamaLLMProvider; + @Autowired + private GenericContainerFactory genericContainerFactory; + @Test public void draftGeneratesValidFlow() { mockAssistantResponses(); @@ -440,6 +446,120 @@ public class AssistantControllerTest { assertEquals("Done", fixedConfiguration.getOutcomeLabel()); } + @Test + public void fixSkipsPlanAndConnectionsForBlockScopedErrorWhenFlowHasContainers() { + LLMDescriptor llmDescriptor = LLMDescriptor.builder().provider("InternalOllama").model(MODEL).build(); + + LLMBlockConfiguration classifierConfiguration = LLMBlockConfiguration.builder() + .name("Ticket classifier") + .llmDescriptor(llmDescriptor) + .prompt("Classify the ticket: ${{ticket}}") + .build(); + Block classifierBlock = Block.builder() + .specificConfiguration(classifierConfiguration) + .input(IODescriptor.of("ticket", IOType.TEXT)) + .output(IODescriptor.of("response", IOType.TEXT)) + .type(new LLMBlockType()) + .build(); + + LLMBlockConfiguration innerConfiguration = LLMBlockConfiguration.builder() + .name("Summarize") + .llmDescriptor(llmDescriptor) + .prompt("Summarize: ${{classification}}") + .build(); + Block innerBlock = Block.builder() + .specificConfiguration(innerConfiguration) + .input(IODescriptor.of("classification", IOType.TEXT)) + .output(IODescriptor.of("response", IOType.TEXT)) + .type(new LLMBlockType()) + .build(); + Container container = + genericContainerFactory.create(GenericContainerConfiguration.builder() + .name("Review") + .subFlow(FlowData.builder().block(innerBlock).build()) + .build()); + + EndBlockConfiguration endConfiguration = EndBlockConfiguration.builder() + .name("Done") + .outcomeCode("DONE") + .outcomeLabel("x".repeat(300)) + .build(); + Block endBlock = Block.builder() + .specificConfiguration(endConfiguration) + .input(IODescriptor.of("input", IOType.ANY)) + .type(new EndBlockType()) + .build(); + + FlowCreateRequest brokenFlow = new FlowCreateRequest( + "Ticket classification with review and terminal outcome", + "Classify a ticket, summarize it in a review container, then record a terminal outcome.", + FlowData.builder() + .block(classifierBlock) + .block(endBlock) + .container(container) + .connection(Connection.builder() + .sourceId(classifierBlock.getId()) + .sourceName("response") + .targetId(container.getId()) + .targetName("classification") + .build()) + .connection(Connection.builder() + .sourceId(container.getId()) + .sourceName("response") + .targetId(endBlock.getId()) + .targetName("input") + .build()) + .build()); + + // Only mocking BLOCK_CONFIG for the flagged (top-level, non-container) block: if targeted + // repair incorrectly bailed out to a full replan because the flow has a container, or + // dropped the container while rebuilding the plan, this mock's fallback would throw or + // the container assertions below would fail. + Answer answer = invocation -> { + String prompt = invocation.getArgument(1, String.class); + if (prompt.contains("TASK: BLOCK_CONFIG")) { + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "Shortened the overlong outcome label.", + "block", java.util.Map.of( + "blockId", endBlock.getId(), + "name", "Done", + "config", java.util.Map.of( + "outcomeCode", "DONE", + "outcomeLabel", "Done")))); + } + throw new IllegalStateException("Unexpected assistant prompt (expected only BLOCK_CONFIG):\n" + prompt); + }; + Mockito.when(internalOllamaLLMProvider.generate(Mockito.eq(MODEL), Mockito.anyString())).thenAnswer(answer); + Mockito.when(internalOllamaLLMProvider.generateJson(Mockito.eq(MODEL), Mockito.anyString())).thenAnswer(answer); + + java.util.List validationErrors = java.util.List.of(new ValidationError( + ValidationErrorCode.VALIDATION_ERROR, "block", endBlock.getId(), + "specificConfiguration.outcomeLabel", "size must be between 0 and 255")); + + AssistantFlowResponse response = assistantController.fix( + new AssistantFixRequest("Fix the overlong outcome label", brokenFlow, validationErrors, MODEL, 1)); + + assertTrue(response.valid(), () -> "Unexpected validation errors: " + response.validationErrors()); + assertTrue(response.validationErrors().isEmpty()); + assertEquals(2, response.flow().flow().getBlocks().size()); + assertEquals(1, response.flow().flow().getContainers().size()); + assertEquals(2, response.flow().flow().getConnections().size()); + + // The container must survive untouched: same id, same inner subflow. + Container resultContainer = response.flow().flow().getContainers().getFirst(); + assertEquals(container.getId(), resultContainer.getId()); + assertEquals(1, resultContainer.getSpecificConfiguration().getSubFlow().getBlocks().size()); + assertEquals("Summarize", + resultContainer.getSpecificConfiguration().getSubFlow().getBlocks().getFirst().getName()); + + Block fixedEndBlock = response.flow().flow().getBlocks().stream() + .filter(block -> "Done".equals(block.getName())) + .findFirst() + .orElseThrow(); + EndBlockConfiguration fixedConfiguration = (EndBlockConfiguration) fixedEndBlock.getSpecificConfiguration(); + assertEquals("Done", fixedConfiguration.getOutcomeLabel()); + } + @Test public void explainReturnsNarrativeText() { mockAssistantResponses();