From fad1eaf4b460c1afc40198bde2add11d46c2f342 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Mon, 3 Aug 2026 15:36:10 +0200 Subject: [PATCH] fix(assistant): stop duplicating loop-body logic at the top level; retry a misplaced container type instead of 502ing Two more issues found retesting the same live prompt after the guardSubFlow-redaction fix (confirmed working: no more guard-scaffold names leaking into prompts). 1. The model kept declaring the same logical step both as a top-level block AND as a container's own inner block (e.g. "check completion" as both b4 and c1-b1), then tried to wire the orphaned top-level duplicate to the container via malformed connections (dotted qualified names, null toInput) - all silently dropped as invalid, but leaving the duplicate disconnected/deadlocked instead of fixing the real problem. Added an explicit NO DUPLICATION rule to the plan prompt: a step belongs in exactly one place, top-level or inside one container, never both - only the container's own exposed I/O is what the rest of the flow should connect to. 2. Separately, a fresh model response put a container type ("LoopContainer") as an entry in the flat "blocks" list instead of "containers". Block assembly has no catalog descriptor for a container type, so this threw as an unrecoverable 502 with no retry - unlike degenerate JSON or a missing required field, which already get a structured-repair retry. Moved the check into validateAndNormalizePlan, inside the plan's own retry-wrapped parser callback, so this now gets the same self-correction chance instead of hard-failing the whole request. Verified by mutation testing: reverting the container-type check reproduces the exact live 502 in the new test; restoring it fixes it. Full suite: 441/441. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../assistant/FlowAssistantPromptService.java | 1 + .../assistant/FlowAssistantService.java | 11 ++++ .../controllers/AssistantControllerTest.java | 52 +++++++++++++++++++ 3 files changed, 64 insertions(+) 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