diff --git a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantPromptService.java b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantPromptService.java index 79bf72d..253d304 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantPromptService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantPromptService.java @@ -158,6 +158,7 @@ public class FlowAssistantPromptService { - For KEEP blocks/containers, reuse the current id/name when available and do not imply configuration changes. - "containers" is optional; omit it (or leave it empty) unless grouping is actually useful. - GROUPING RULE: put a cohesive group of blocks (e.g. 3+ steps that form one logical phase) inside a "containers" entry instead of leaving them all as flat top-level blocks, when doing so makes the flow's overall structure clearer - especially for flows with many steps across several logical phases. A container's own "blocks" list uses the exact same block shape as top-level blocks (blockId/blockType/purpose), and never put another container inside a container (nesting is not supported) - a container always needs at least one inner block. + - NO DUPLICATION: a given logical step belongs in exactly ONE place - either as a top-level block, or as one inner block of one container - never both. If a step's purpose is to run inside a container's repeated/grouped body (e.g. "revise the draft", "check completeness" as part of a LoopContainer's iteration), do NOT also add a separate top-level block for that same purpose. Only the container's own exposed input/output (derived automatically from whichever inner handles are left open) is what the rest of the top-level flow connects to - never re-declare the container's inner logic again at the top level. - When REFINE/FIX must change an existing container, mark the container UPDATE and, in its "blocks" list, set each inner block's "operation": KEEP for inner blocks that stay unchanged (reuse their existing id/name so they are matched and reused verbatim), ADD for new inner blocks, UPDATE for inner blocks whose config changes, REMOVE for inner blocks to delete. Include every inner block that should remain, even the KEEP ones. For a brand-new (ADD) container, or for DRAFT, omit inner "operation" (all inner blocks are created). - containerType must be exactly "GenericContainer", "IteratorContainer" or "LoopContainer"; do not invent other container types. - Use "GenericContainer" for a plain one-shot grouping of steps (no repetition). Use "IteratorContainer" when the same subflow must run once per element of a list whose length is data, not known at design time ("for each candidate/document/ticket, do X"). Use "LoopContainer" when the same subflow must run REPEATEDLY on the same item until a condition is met ("keep revising the draft until it passes review", "retry until valid"). Do NOT use an IteratorContainer/LoopContainer just to group steps. 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 5db7d6e..973add6 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 @@ -2236,6 +2236,17 @@ public class FlowAssistantService { throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, "Assistant returned a block without blockType"); } + if (isContainerBlockType(block.blockType())) { + // A container type placed in the flat "blocks" list (instead of "containers") used + // to fail hard as an unrecoverable 502 - the block-assembly loop has no catalog + // descriptor for a container type and never gets a chance to retry. Thrown here, + // inside the plan's own retry-wrapped parser callback, it becomes a normal + // structured-repair retry instead. + throw new ResponseStatusException(HttpStatus.BAD_GATEWAY, + "Assistant placed a container type '" + block.blockType() + "' (blockId " + block.blockId() + + ") in the top-level \"blocks\" list - container types must be entries in the" + + " \"containers\" list instead, with their own inner \"blocks\""); + } if (isSharedMemoryContext(userPrompt, currentFlow, plan) && "LLMBlock".equals(block.blockType()) && isSharedStatePurpose(block.purpose())) { 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 49703e3..7ab130d 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 @@ -3137,6 +3137,58 @@ public class AssistantControllerTest { assertEquals(2, response.flow().flow().getBlocks().size()); } + @Test + public void draftRetriesInsteadOf502WhenModelPutsAContainerTypeInTopLevelBlocks() { + // Live incident: the model sometimes puts a container type (e.g. "LoopContainer") as an + // entry in the flat "blocks" list instead of "containers". Block assembly has no catalog + // descriptor for a container type, so this used to escape as an unrecoverable 502 with no + // chance to self-correct. It must now be caught inside the plan's own retry-wrapped parser, + // so a structured-repair retry gets a chance to fix it - same mechanism already used for a + // degenerate empty plan or a missing required field. + Answer answer = invocation -> { + String prompt = invocation.getArgument(1, String.class); + if (prompt.contains("TASK: PLAN") && prompt.contains("The previous response did not satisfy")) { + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "Corrected: the loop belongs in containers, not blocks.", + "plan", java.util.Map.of("name", "Fixed plan", "description", "desc", + "blocks", java.util.List.of(), + "containers", java.util.List.of(java.util.Map.of( + "containerId", "c1", "containerType", "LoopContainer", + "purpose", "Revise until clean", "operation", "ADD", + "guardCondition", "stop when clean", "maxIterations", 5, + "blocks", java.util.List.of(java.util.Map.of("blockId", "c1-b1", + "blockType", "LLMBlock", "purpose", "Revise the draft"))))))); + } + if (prompt.contains("TASK: PLAN")) { + // First attempt: the model misplaces a container type inside "blocks". + return TestAssistantResponses.wrap(java.util.Map.of( + "rationale", "A loop that revises the draft.", + "plan", java.util.Map.of("name", "Bad plan", "description", "desc", + "blocks", java.util.List.of(java.util.Map.of( + "blockId", "b1", "blockType", "LoopContainer", + "purpose", "Revise until clean", "operation", "ADD")), + "containers", java.util.List.of()))); + } + if (prompt.contains("TASK: BLOCK_CONFIG")) { + return TestAssistantResponses.wrap(java.util.Map.of("rationale", "revise", + "block", java.util.Map.of("blockId", "c1-b1", "name", "revise-draft", + "config", java.util.Map.of("prompt", "Revise this draft: ${{draft}}")))); + } + throw new IllegalStateException("Unexpected assistant prompt:\n" + prompt); + }; + Mockito.when(internalOllamaLLMProvider.generate(Mockito.eq(MODEL), Mockito.anyString())).thenAnswer(answer); + Mockito.when(internalOllamaLLMProvider.generateJson(Mockito.eq(MODEL), Mockito.anyString())).thenAnswer(answer); + + AssistantFlowResponse response = assistantController.draft( + new AssistantGenerationRequest("keep revising the draft until it is clean", MODEL, 1)); + + assertNotNull(response); + assertTrue(response.valid(), () -> "Unexpected validation errors: " + response.validationErrors()); + assertEquals(0, response.flow().flow().getBlocks().size()); + assertEquals(1, response.flow().flow().getContainers().size()); + assertEquals("LoopContainer", response.flow().flow().getContainers().getFirst().getType().getName()); + } + @Test public void draftDefaultsMissingRequiredTextFieldInsteadOf502() { // MCPAgentChatBlockConfiguration requires goalDescription; if the model omits it, the