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

feat: validate assistant flows with FlowExecutionValidator, raise repair budget to 2 

The assistant's validate step only ran jakarta bean-validation on
FlowCreateRequest, which cascades into FlowDataValidator (the
@ValidFlowStructure constraint on FlowData) but never into each
block's own specificConfiguration - Block.specificConfiguration has
no @Valid annotation. So per-block constraints (BranchRejoinBlockConfiguration's
2-10 input bound, LLMBlockConfiguration's placeholder-name regex, MCP
shared-session producer/consumer wiring, container subflow rules via
the execution-level checks, ...) were never actually enforced: the
assistant could emit a structurally-broken block and still report
valid=true.

- FlowAssistantService.validate() now also runs
  FlowExecutionValidator.collectErrors(), merging its errors with the
  existing bean-validation errors so the repair loop sees (and can fix)
  what used to slip through silently.
- Default max repair attempts raised from 1 to 2 (still capped at the
  existing max of 3), since a single fix round often isn't enough once
  more error classes are actually being caught.
- Fixed draftDoesNotFailWhenSharedMemoryFlagsAreIncomplete: its flow was
  already broken (two MCP consumer-only blocks referencing a shared
  session neither produces) and is now correctly reported invalid
  instead of a false-positive valid=true.
- Added two regression tests locking in the new coverage and the new
  default repair budget.
This commit is contained in:
Lucio Lelii 2026-07-25 12:31:07 +02:00
parent 9556779241
commit 7cabf6aa08
2 changed files with 105 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -43,6 +43,7 @@ import it.cnr.isti.workflow.manager.blocks.factories.BlockFactory;
import it.cnr.isti.workflow.manager.flows.model.Connection;
import it.cnr.isti.workflow.manager.flows.model.FlowCreateRequest;
import it.cnr.isti.workflow.manager.flows.model.FlowData;
import it.cnr.isti.workflow.manager.flows.validation.FlowExecutionValidator;
import it.cnr.isti.workflow.manager.flows.validation.ValidationError;
import it.cnr.isti.workflow.manager.flows.validation.ValidationErrorCodec;
import it.cnr.isti.workflow.manager.ios.IODescriptor;
@ -59,6 +60,7 @@ public class FlowAssistantService {
private static final String INTERNAL_PROVIDER_NAME = "InternalOllama";
private static final String SHARED_MEMORY_SESSION_NAME = "sharedMemorySession";
private static final int DEFAULT_PROVIDER_RETRY_ATTEMPTS = 3;
private static final int DEFAULT_MAX_REPAIR_ATTEMPTS = 2;
private static final long DEFAULT_RETRY_BASE_DELAY_MILLIS = 120L;
private static final long DEFAULT_RETRY_MAX_DELAY_MILLIS = 800L;
private static final ObjectMapper LENIENT_ASSISTANT_MAPPER = JsonMapper.builder()
@ -136,6 +138,9 @@ public class FlowAssistantService {
@Autowired
private Validator validator;
@Autowired
private FlowExecutionValidator flowExecutionValidator;
@Value("${app.assistant.provider-retry-base-delay-ms:" + DEFAULT_RETRY_BASE_DELAY_MILLIS + "}")
private long retryBaseDelayMillis;
@ -224,7 +229,7 @@ public class FlowAssistantService {
Integer maxRepairAttempts, ProgressListener progressListener) {
LLMProvider provider = resolveInternalProvider();
ResolvedAssistantModels phaseModels = resolveAssistantModels(workflowModel, requestedPhaseModels);
int allowedRepairs = maxRepairAttempts == null ? 1 : maxRepairAttempts;
int allowedRepairs = maxRepairAttempts == null ? DEFAULT_MAX_REPAIR_ATTEMPTS : maxRepairAttempts;
int repairs = 0;
OperationMode mode = initialMode;
FlowCreateRequest flowContext = currentFlow;
@ -1625,12 +1630,9 @@ public class FlowAssistantService {
}
private List<ValidationError> validate(FlowCreateRequest flow) {
Set<ConstraintViolation<FlowCreateRequest>> violations = validator.validate(flow);
if (violations.isEmpty()) {
return List.of();
}
List<ValidationError> errors = new ArrayList<>();
Set<ConstraintViolation<FlowCreateRequest>> violations = validator.validate(flow);
for (ConstraintViolation<FlowCreateRequest> violation : violations) {
List<ValidationError> decoded = ValidationErrorCodec.decode(violation.getMessage());
if (decoded.isEmpty()) {
@ -1645,6 +1647,13 @@ public class FlowAssistantService {
}
}
}
// Bean validation alone misses structural/execution issues (dangling connections,
// an unconnected BranchRejoin input, global-input reference mismatches, container
// subflow rules, ...) that only surface at actual execution time otherwise.
if (flow != null && flow.flow() != null) {
errors.addAll(flowExecutionValidator.collectErrors(flow.flow()));
}
return errors;
}

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

@ -573,6 +573,12 @@ public class AssistantControllerTest {
@Test
public void draftDoesNotFailWhenSharedMemoryFlagsAreIncomplete() {
// "Does not fail" means draft() still returns a response instead of throwing, even
// though this flow is genuinely broken (both blocks are consumer-only, so neither
// produces the shared session they reference). FlowExecutionValidator now catches
// that as an execution-level error - previously only bean-validation ran here, which
// doesn't check MCP shared-session producer/consumer wiring, so this used to be
// reported as valid=true despite never being executable.
mockSharedMemoryResponsesWithIncompleteFlags();
AssistantFlowResponse response = assistantController.draft(
@ -582,7 +588,8 @@ public class AssistantControllerTest {
1));
assertNotNull(response);
assertTrue(response.valid());
assertFalse(response.valid());
assertFalse(response.validationErrors().isEmpty());
assertEquals(2, response.flow().flow().getBlocks().size());
}
@ -1536,6 +1543,88 @@ public class AssistantControllerTest {
throw new AssertionError("Assistant call did not complete in time");
}
@Test
public void draftCatchesConfigurationConstraintNotCoveredByStructuralValidation() {
// FlowDataValidator (bean-validation cascade) never validates a block's own
// specificConfiguration constraints; only FlowExecutionValidator does, per-block.
// Before wiring FlowExecutionValidator into the assistant's validate step, an invalid
// placeholder name here would have silently passed as valid=true.
mockBlockConfigWithInvalidPlaceholderThenValidAfter(1);
AssistantFlowResponse response = assistantController.draft(
new AssistantGenerationRequest(
"create a flow that greets a person by name",
MODEL,
1));
assertNotNull(response);
assertTrue(response.valid());
assertEquals(1, response.repairAttempts());
LLMBlockConfiguration configuration = (LLMBlockConfiguration) response.flow().flow().getBlocks().getFirst()
.getSpecificConfiguration();
assertEquals("Greet ${{name}}", configuration.getPrompt());
}
@Test
public void draftDefaultsToTwoRepairAttemptsWhenNotSpecified() {
// Needs two repair rounds to succeed - only possible now that the default repair
// budget is 2 instead of 1.
mockBlockConfigWithInvalidPlaceholderThenValidAfter(2);
AssistantFlowResponse response = assistantController.draft(
new AssistantGenerationRequest(
"create a flow that greets a person by name",
MODEL,
null));
assertNotNull(response);
assertTrue(response.valid());
assertEquals(2, response.repairAttempts());
}
private void mockBlockConfigWithInvalidPlaceholderThenValidAfter(int validAfterAttempt) {
AtomicInteger blockConfigAttempts = new AtomicInteger();
Answer<String> answer = invocation -> {
String prompt = invocation.getArgument(1, String.class);
if (prompt.contains("TASK: PLAN")) {
return TestAssistantResponses.wrap(java.util.Map.of(
"rationale", "Planned a single greeting step.",
"plan", java.util.Map.of(
"name", "Greeting flow",
"description", "Greet a person by name.",
"blocks", java.util.List.of(
java.util.Map.of(
"blockId", "b1",
"blockType", "LLMBlock",
"purpose", "Greet the person")))));
}
if (prompt.contains("TASK: BLOCK_CONFIG")) {
if (blockConfigAttempts.incrementAndGet() <= validAfterAttempt) {
return TestAssistantResponses.wrap(java.util.Map.of(
"rationale", "Configured the greeting block with an invalid placeholder name.",
"block", java.util.Map.of(
"blockId", "b1",
"name", "Greeter",
"config", java.util.Map.of(
"prompt", "Greet ${{bad name}}"))));
}
return TestAssistantResponses.wrap(java.util.Map.of(
"rationale", "Configured the greeting block.",
"block", java.util.Map.of(
"blockId", "b1",
"name", "Greeter",
"config", java.util.Map.of(
"prompt", "Greet ${{name}}"))));
}
throw new IllegalStateException("Unexpected assistant prompt:\n" + prompt);
};
Mockito.when(internalOllamaLLMProvider.generate(Mockito.anyString(), Mockito.anyString()))
.thenAnswer(answer);
Mockito.when(internalOllamaLLMProvider.generateJson(Mockito.anyString(), Mockito.anyString()))
.thenAnswer(answer);
}
static class TestAssistantResponses {
private static final String PROVIDER = "InternalOllama";