From ab1def92f191579d6b6ff78ff09c88ebc62c3de8 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Sat, 25 Jul 2026 16:59:15 +0200 Subject: [PATCH] feat: allow targeted repair on flows that contain containers isTargetedBlockRepairEligible previously bailed out to a full replan whenever currentFlow had any containers at all, because buildReusedPlanForTargetedRepair only reconstructed the top-level block list - running it on a flow with containers would have silently dropped them from the rebuilt plan. buildReusedPlanForTargetedRepair now also reconstructs an AssistantContainerPlan (operation KEEP, empty inner blocks) for every container in currentFlow.flow().getContainers(), so containers survive the targeted-repair path unchanged instead of disappearing. The eligibility check still requires every error to be scoped to an existing top-level block - an error scoped to a container (or anything else) still falls back to a full replan, since a KEEP-only container plan can't fix a container-internal problem. Added a regression test: a flow with a container present has a top-level block's overlong EndBlock.outcomeLabel fixed via a single targeted BLOCK_CONFIG call (PLAN/CONNECTIONS both skipped), and the container comes back byte-for-byte identical (same id, same inner subflow). Updated docs/assistant-completion-roadmap-2026-07-25.md to mark this item done and note what's still open (container-internal errors are still not targeted-fixable - that's the separate, larger "incremental container diffing" item). --- .../assistant/FlowAssistantService.java | 27 +++- .../controllers/AssistantControllerTest.java | 120 ++++++++++++++++++ 2 files changed, 141 insertions(+), 6 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 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();