lelii
/
humainflow-service
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(assistant): stop resurrecting stale connections across forced-regen repair rounds 

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) <noreply@anthropic.com>
This commit is contained in:
Lucio Lelii 2026-08-03 13:13:20 +02:00
parent 66112541a5
commit 3d16a3b388
2 changed files with 105 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java
View File

@ -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);

89
src/test/java/it/cnr/isti/workflow/manager/controllers/AssistantControllerTest.java
View File

@ -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<LLMBlockType> reviewer = Block.<LLMBlockType>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<LLMBlockType> reviser = Block.<LLMBlockType>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<GenericContainerType> 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<String> 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<ValidationError> 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<Connection> 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