From 3d16a3b388eb9e580995b3b0d96ff157fcb7c85e Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Mon, 3 Aug 2026 13:13:20 +0200 Subject: [PATCH] fix(assistant): stop resurrecting stale connections across forced-regen repair rounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answers "perché il fix non riesce a risolvere il problema?" - traced via a live repro (session log + direct save attempt) why a LoopContainer body left in a fully closed 2-block cycle (no input left open for guard feedback) never got fixed even after both repair rounds ran. Root cause: when a container-level validation error isn't attributable to any specific inner block, normalizeContainerInnerBlocksAgainstExisting force-ADDs every inner block each round (the existing "a FIX round that changes nothing would never fix the error" fallback) - even though the model marks them KEEP. But resolveExistingBlock's position-based fallback still matches the old block regardless of the forced operation, so oldInnerIdToAssembled/oldNodeIdToAssembledNode got populated anyway, and preserveConnections carried the OLD (still-broken) connections forward every round on top of whatever the fresh connections-for- container call produced. The model has no way to ask for a connection's removal, so the stale, well-formed (non-dangling) connection survived indefinitely - the previous dangling-reference fixes didn't catch this because nothing here is dangling, it's a stale-but-valid connection. Fix: only populate the old-to-new node id mapping used for connection preservation when the block/container's operation is genuinely KEEP or UPDATE, never ADD - regardless of why it became ADD (explicit model choice or the force-regen fallback). Applied consistently at all three sites (top-level blocks, top-level containers, container-inner blocks). Verified by mutation testing: reverting the container-inner guard reproduces the exact live failure (2 connections instead of 1) in the new regression test; restoring it fixes it. Full suite: 439/439. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../assistant/FlowAssistantService.java | 19 +++- .../controllers/AssistantControllerTest.java | 89 +++++++++++++++++++ 2 files changed, 105 insertions(+), 3 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 b2724d1..5db7d6e 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 @@ -493,7 +493,12 @@ public class FlowAssistantService { assembledBlocks.add(assembledBlock); nodesByPlanId.put(blockPlan.blockId(), assembledBlock); - if (existingBlock != null) { + // Only a genuine KEEP/UPDATE carries its old connections forward. An ADD block is a new + // entity even when resolveExistingBlock incidentally position-matched an old one (e.g. a + // container-error full-regen that force-ADDs every inner block despite the model saying + // KEEP) - treating that coincidental match as "same entity" would resurrect old + // connections the model never asked to keep and has no way to countermand. + if (existingBlock != null && operation != PlanOperation.ADD) { oldNodeIdToAssembledNode.put(existingBlock.getId(), assembledBlock); } registerNodeAlias(nodesByAlias, blockPlan.blockId(), assembledBlock); @@ -543,7 +548,7 @@ public class FlowAssistantService { assembledContainers.add(assembledContainer); nodesByPlanId.put(containerPlan.containerId(), assembledContainer); - if (existingContainer != null) { + if (existingContainer != null && containerOperation != PlanOperation.ADD) { oldNodeIdToAssembledNode.put(existingContainer.getId(), assembledContainer); } registerNodeAlias(nodesByAlias, containerPlan.containerId(), assembledContainer); @@ -689,7 +694,15 @@ public class FlowAssistantService { innerBlocks.add(assembledBlock); innerNodesByPlanId.put(blockPlan.blockId(), assembledBlock); - if (existingInnerBlock != null) { + // Only a genuine KEEP carries its old connections forward (see the matching top-level + // guard above). This matters most here: when a container-level error isn't attributable + // to any inner block, normalizeContainerInnerBlocksAgainstExisting force-ADDs every inner + // block even though the model marked them KEEP (see its "all-KEEP forces regen" + // fallback) - without this guard, resolveExistingBlock's position-based fallback still + // matches the old block, and the OLD (possibly still-broken) inner connections would be + // reincarnated every repair round no matter what the fresh connections-for-container + // call produces, since the model has no way to ask for a connection's removal. + if (existingInnerBlock != null && operation != PlanOperation.ADD) { oldInnerIdToAssembled.put(existingInnerBlock.getId(), assembledBlock); } registerNodeAlias(innerNodesByAlias, blockPlan.blockId(), assembledBlock); 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 07588a1..b089089 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 @@ -2755,6 +2755,95 @@ public class AssistantControllerTest { assertEquals("payload", subFlowConnections.getFirst().getTargetName()); } + @Test + public void fixDoesNotResurrectStaleWellFormedConnectionWhenAllInnerBlocksForcedToRegen() { + // Live incident: a LoopContainer body wired as a fully closed 2-block cycle (every input fed + // internally, none left open for guard feedback) never gets fixed across repair rounds. The + // container-level error force-regens every inner block each round (same all-KEEP fallback as + // the sibling test above), and resolveExistingBlock's position-based fallback still matches + // the old blocks even though they're now ADD - so oldInnerIdToAssembled got populated and + // preserveConnections carried the OLD closed loop forward every round. Unlike the sibling + // test's stale connection, this old connection is NOT dangling (its name never changed), so + // the name-based guard in preserveConnections doesn't catch it - only NOT treating a + // force-ADD block as "the same entity" for connection provenance does. Each round's own fresh + // "connections for container c1" call correctly generates only ONE direction (leaving an + // input open); the bug was the OTHER (stale) direction being reincarnated on top of it. + LLMDescriptor llmDescriptor = LLMDescriptor.builder().provider("InternalOllama").model(MODEL).build(); + Block reviewer = Block.builder() + .specificConfiguration(LLMBlockConfiguration.builder().name("review").llmDescriptor(llmDescriptor) + .prompt("Review: ${{draft}}").build()) + .input(IODescriptor.of("draft", IOType.TEXT)).output(IODescriptor.of("verdict", IOType.TEXT)) + .type(new LLMBlockType()).build(); + Block reviser = Block.builder() + .specificConfiguration(LLMBlockConfiguration.builder().name("revise").llmDescriptor(llmDescriptor) + .prompt("Revise: ${{feedback}}").build()) + .input(IODescriptor.of("feedback", IOType.TEXT)).output(IODescriptor.of("response", IOType.TEXT)) + .type(new LLMBlockType()).build(); + Container container = genericContainerFactory.create( + GenericContainerConfiguration.builder().name("Revise loop body").subFlow(FlowData.builder() + .block(reviewer).block(reviser) + // The fully closed loop: every input is fed internally, nothing open. + .connection(Connection.builder().sourceId(reviewer.getId()).sourceName("verdict") + .targetId(reviser.getId()).targetName("feedback").build()) + .connection(Connection.builder().sourceId(reviser.getId()).sourceName("response") + .targetId(reviewer.getId()).targetName("draft").build()) + .build()).build()); + FlowCreateRequest brokenFlow = new FlowCreateRequest("Loop body", "desc", + FlowData.builder().container(container).build()); + + Answer answer = invocation -> { + String prompt = invocation.getArgument(1, String.class); + if (prompt.contains("TASK: PLAN")) { + return TestAssistantResponses.wrap(java.util.Map.of("rationale", "Leave one input open.", + "plan", java.util.Map.of("name", "Loop body", "description", "desc", + "blocks", java.util.List.of(), + "containers", java.util.List.of(java.util.Map.of( + "containerId", container.getId(), "containerType", "GenericContainer", + "purpose", "Revise loop body", "operation", "UPDATE", + "blocks", java.util.List.of( + java.util.Map.of("blockId", reviewer.getId(), "blockType", "LLMBlock", + "purpose", "Review the draft", "operation", "KEEP"), + java.util.Map.of("blockId", reviser.getId(), "blockType", "LLMBlock", + "purpose", "Revise the draft", "operation", "KEEP"))))))); + } + if (prompt.contains("TASK: BLOCK_CONFIG") + && prompt.contains("Current block to configure:\n{\n \"blockId\" : \"" + reviewer.getId() + "\"")) { + return TestAssistantResponses.wrap(java.util.Map.of("rationale", "unchanged", + "block", java.util.Map.of("blockId", reviewer.getId(), "name", "review", + "config", java.util.Map.of("prompt", "Review: ${{draft}}")))); + } + if (prompt.contains("TASK: BLOCK_CONFIG") + && prompt.contains("Current block to configure:\n{\n \"blockId\" : \"" + reviser.getId() + "\"")) { + return TestAssistantResponses.wrap(java.util.Map.of("rationale", "unchanged", + "block", java.util.Map.of("blockId", reviser.getId(), "name", "revise", + "config", java.util.Map.of("prompt", "Revise: ${{feedback}}")))); + } + if (prompt.contains("TASK: CONNECTIONS")) { + // This round's own generation correctly leaves "draft" open - only one direction. + return TestAssistantResponses.wrap(java.util.Map.of("rationale", "one direction only", + "connections", java.util.List.of(java.util.Map.of("fromBlockId", reviewer.getId(), + "fromOutput", "verdict", "toBlockId", reviser.getId(), "toInput", "feedback")))); + } + 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); + + java.util.List errors = java.util.List.of(new ValidationError( + ValidationErrorCode.CONTAINER_SUBFLOW_INVALID, "container", container.getId(), + "specificConfiguration.subFlow", "must expose an open input")); + + AssistantFlowResponse response = assistantController.fix( + new AssistantFixRequest("fix the loop body", brokenFlow, errors, MODEL, 1)); + + assertNotNull(response); + Container result = response.flow().flow().getContainers().getFirst(); + java.util.List subFlowConnections = result.getSpecificConfiguration().getSubFlow().getConnections(); + assertEquals(1, subFlowConnections.size(), + "the stale closed-loop connection must not be reincarnated: " + subFlowConnections); + assertEquals("feedback", subFlowConnections.getFirst().getTargetName()); + } + @Test public void fixDropsMismatchedFeedbackInputAndLetsInferenceResolveIt() { // The model may guess a feedbackInput name (e.g. reusing an example placeholder name from