From 484de60852e5ce5283ac24c5dc8b80efe06d9d3d Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Mon, 3 Aug 2026 16:16:31 +0200 Subject: [PATCH] feat(flows): make saving permissive - not-yet-executable flows persist as DRAFT The categorical fix for the recurring "assistant produced a flow I can't save" problem. Until now, POST/PUT /flows rejected (400) on ANY @ValidFlowStructure violation, conflating two very different things: genuinely corrupt/inconsistent data, and a flow that is merely not runnable yet. The user's model - and how workflow editors normally behave - is that an incomplete flow must be savable as a DRAFT and only gated at execution time. Key realisation: FlowExecutionValidator.collectErrors already runs the same @ValidFlowStructure bean validation, so DRAFT vs EXECUTABLE status (toView -> isExecutable) already reflects every structural/executability problem, and ExecutionsService.startExecution independently calls flowExecutionValidator.validate() - so a non-executable draft can never actually run. The hard save-gate was therefore redundant for the executability class; only data-integrity needed to keep blocking. FlowService.validateFlow now partitions violations by code: - SAVE_BLOCKING_CODES (integrity: type/inputs/outputs mismatch, missing config, duplicate/missing node ids, unknown node type, nested containers, lane integrity, global-input integrity, bias-annotation integrity, and non-decodable request-level constraints like a null flow) still reject with 400, re-encoded via ValidationErrorCodec so the structured errors[] contract is unchanged. - everything else (dangling/absent connections, container subflow not yet exposing its handles, exclusive-branch merges, branch-rejoin/end gaps, dependencies, deadlocks, shared-session ordering, ...) no longer blocks: the flow saves as DRAFT and the issue is surfaced by GET /flows/{id}/validation. This closes the whole class of "structurally sane but not runnable -> can't save" failures once and for all, instead of chasing each variant. Tests: dangling connection now saves as DRAFT (was 400); the two exclusive-branch-merge tests updated from "rejected" to "saved as draft, reported by the execution validator"; factory-tampering and bias-integrity rejections still 400 unchanged. 443/443. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflow/manager/flows/FlowService.java | 83 +++++++++++++++++-- .../controllers/FlowControllerTest.java | 83 +++++++++++++++---- 2 files changed, 146 insertions(+), 20 deletions(-) diff --git a/src/main/java/it/cnr/isti/workflow/manager/flows/FlowService.java b/src/main/java/it/cnr/isti/workflow/manager/flows/FlowService.java index 8b8884b..4a1ac67 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/flows/FlowService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/flows/FlowService.java @@ -15,11 +15,14 @@ import it.cnr.isti.workflow.manager.flows.repo.FlowEntity; import it.cnr.isti.workflow.manager.flows.repo.FlowRepository; 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.ValidationErrorCode; +import it.cnr.isti.workflow.manager.flows.validation.ValidationErrorCodec; import jakarta.validation.ConstraintViolation; import jakarta.validation.Validator; +import java.util.ArrayList; +import java.util.EnumSet; import java.util.Set; -import java.util.stream.Collectors; @Service public class FlowService { @@ -133,17 +136,85 @@ public class FlowService { return flowExecutionValidator.collectErrors(entity.getFlow()); } + /** + * Data-integrity errors that genuinely block a save: malformed, internally inconsistent, or + * factory-tampered structure that could not be trusted or safely reloaded. Everything NOT in + * this set is treated as "not yet executable" rather than "corrupt" - see {@link #validateFlow}. + * VALIDATION_ERROR is here because at the save gate it only ever comes from integrity checks + * (null/blank/duplicate global input, null/invalid lane) or a non-decodable request-level + * constraint (e.g. the flow being null); block-config bean violations are NOT cascaded here. + */ + private static final Set SAVE_BLOCKING_CODES = EnumSet.of( + ValidationErrorCode.VALIDATION_ERROR, + ValidationErrorCode.FLOW_CONTAINS_NULL_NODE, + ValidationErrorCode.NODE_ID_REQUIRED, + ValidationErrorCode.DUPLICATE_NODE_ID, + ValidationErrorCode.UNSUPPORTED_NODE_TYPE, + ValidationErrorCode.BLOCK_CONFIGURATION_MISSING, + ValidationErrorCode.BLOCK_TYPE_MISMATCH, + ValidationErrorCode.BLOCK_INPUTS_MISMATCH, + ValidationErrorCode.BLOCK_OUTPUTS_MISMATCH, + ValidationErrorCode.CONTAINER_CONFIGURATION_MISSING, + ValidationErrorCode.CONTAINER_TYPE_MISMATCH, + ValidationErrorCode.CONTAINER_INPUTS_MISMATCH, + ValidationErrorCode.CONTAINER_OUTPUTS_MISMATCH, + ValidationErrorCode.NESTED_CONTAINERS_NOT_SUPPORTED, + ValidationErrorCode.DUPLICATE_LANE_ID, + ValidationErrorCode.LANE_NAME_REQUIRED, + ValidationErrorCode.NODE_LANE_NOT_FOUND, + ValidationErrorCode.LANE_ORDER_INVALID, + ValidationErrorCode.BIAS_ANNOTATIONS_NOT_ALLOWED, + ValidationErrorCode.TOO_MANY_BIAS_ANNOTATIONS, + ValidationErrorCode.NULL_BIAS_ANNOTATION, + ValidationErrorCode.DUPLICATE_BIAS_ANNOTATION_ID, + ValidationErrorCode.BIAS_CATEGORY_REQUIRED, + ValidationErrorCode.BIAS_SEVERITY_REQUIRED, + ValidationErrorCode.BIAS_ISSUE_REQUIRED, + ValidationErrorCode.BIAS_FIELD_TOO_LONG, + ValidationErrorCode.BIAS_PROBE_MODE_REQUIRED, + ValidationErrorCode.BIAS_PROBE_INSTRUCTION_REQUIRED, + ValidationErrorCode.BIAS_PROBE_MODE_UNSUPPORTED, + ValidationErrorCode.BIAS_PROBE_TARGET_INPUT_NOT_FOUND, + ValidationErrorCode.BIAS_PROBE_MOCK_OUTPUTS_REQUIRED, + ValidationErrorCode.BIAS_PROBE_MOCK_OUTPUT_NOT_FOUND, + ValidationErrorCode.BIAS_PROBE_MOCK_OUTPUT_TYPE_MISMATCH); + + /** + * Permissive save. A flow that is merely NOT YET EXECUTABLE - dangling or absent connections, a + * container subflow that doesn't yet expose the handles it needs, routing/branch/end-node gaps, + * an execution deadlock, ... - must still be storable so it can be iterated in the editor; it is + * simply persisted with status DRAFT (see {@link #toView}). Execution is independently gated by + * {@link FlowExecutionValidator#validate} at run time, so a non-executable draft can never run. + * Only genuine data-integrity problems ({@link #SAVE_BLOCKING_CODES}) reject the save. + */ private void validateFlow(FlowCreateRequest request) { Set> violations = validator.validate(request); if (violations.isEmpty()) { return; } - String message = violations.stream() - .map(violation -> violation.getPropertyPath() + " " + violation.getMessage()) - .collect(Collectors.joining(", ")); - logger.warn("Flow validation failed: {}", message); - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, message); + List blocking = new ArrayList<>(); + List allowedAsDraft = new ArrayList<>(); + for (ConstraintViolation violation : violations) { + for (ValidationError error : ValidationErrorCodec.decode(violation.getMessage())) { + if (SAVE_BLOCKING_CODES.contains(error.code())) { + blocking.add(error); + } else { + allowedAsDraft.add(error); + } + } + } + + if (!blocking.isEmpty()) { + // Keep the reason ValidationErrorCodec-encoded so ApiExceptionHandler surfaces the same + // structured {code, entity, id, field, message} errors[] the clients already consume. + String encoded = ValidationErrorCodec.encode(blocking); + logger.warn("Flow save rejected on data-integrity errors: {}", encoded); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, encoded); + } + if (!allowedAsDraft.isEmpty()) { + logger.debug("Flow saved as draft despite non-executable validation issues: {}", allowedAsDraft); + } } private void ensureMutable(FlowEntity entity) { diff --git a/src/test/java/it/cnr/isti/workflow/manager/controllers/FlowControllerTest.java b/src/test/java/it/cnr/isti/workflow/manager/controllers/FlowControllerTest.java index e3fedb7..20bd379 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/controllers/FlowControllerTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/controllers/FlowControllerTest.java @@ -818,6 +818,52 @@ public class FlowControllerTest { assertEquals(FlowViewStatus.DRAFT, createResponse.getBody().status()); } + @Test + public void createFlowWithDanglingConnectionSavesAsDraftInsteadOfRejecting() { + // A connection whose target input name doesn't exist on the target block is a classic + // "not yet executable" state (e.g. a half-rewired flow, or assistant output mid-iteration). + // It must SAVE as a DRAFT so the user can fix it in the editor - not be rejected at save + // time with a 400 - while the execution validator still reports the dangling connection and + // the flow can never actually run until it is fixed. + LLMDescriptor llmDescriptor = LLMDescriptor.builder() + .provider("testProvider") + .model("testModel") + .build(); + Block source = blocksController.create(LLMBlockConfiguration.builder() + .name("source") + .llmDescriptor(llmDescriptor) + .prompt("Produce output from ${{seed}}") + .build()); + Block target = blocksController.create(LLMBlockConfiguration.builder() + .name("target") + .llmDescriptor(llmDescriptor) + .prompt("Consume ${{expected}}") + .build()); + + FlowCreateRequest request = new FlowCreateRequest( + "Dangling connection flow", + "The connection points at an input the target block does not have", + FlowData.builder() + .block(source) + .block(target) + .connection(Connection.builder() + .sourceId(source.getId()) + .sourceName("response") + .targetId(target.getId()) + .targetName("does_not_exist") + .build()) + .build()); + + FlowView created = flowController.createFlow(request, new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(created); + assertEquals(FlowViewStatus.DRAFT, created.status()); + List validation = flowController + .getFlowValidation(created.id(), new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(validation); + assertTrue(validation.stream().anyMatch(e -> e.code() == ValidationErrorCode.CONNECTION_TARGET_INPUT_NOT_FOUND), + () -> "expected CONNECTION_TARGET_INPUT_NOT_FOUND among execution errors: " + validation); + } + @Test public void createEmptyFlowReturnsDraftStatus() { FlowCreateRequest request = new FlowCreateRequest( @@ -1078,7 +1124,7 @@ public class FlowControllerTest { } @Test - public void createFlowRejectsConditionalBranchMerge() { + public void createFlowSavesConditionalBranchMergeAsDraft() { LLMDescriptor llmDescriptor = LLMDescriptor.builder() .provider("testProvider") .model("testModel") @@ -1156,16 +1202,21 @@ public class FlowControllerTest { .build()) .build()); - ResponseStatusException exception = assertThrows( - ResponseStatusException.class, - () -> flowController.createFlow(request, new LoginEntity("testuser", "testpassword"))); - - assertEquals(HttpStatus.BAD_REQUEST, exception.getStatusCode()); - assertTrue(exception.getReason().contains("Exclusive routing branches must not merge")); + // An exclusive-branch merge is a not-yet-executable ROUTING structure, not corrupt data: + // saving is permissive, so it persists as a DRAFT (fixable in the editor) and the merge is + // surfaced by the execution validator - it is no longer rejected outright at save time. + FlowView created = flowController.createFlow(request, new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(created); + assertEquals(FlowViewStatus.DRAFT, created.status()); + List validation = flowController + .getFlowValidation(created.id(), new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(validation); + assertTrue(validation.stream().anyMatch(e -> e.code() == ValidationErrorCode.EXCLUSIVE_BRANCH_MERGE), + () -> "expected EXCLUSIVE_BRANCH_MERGE among execution errors: " + validation); } @Test - public void createFlowRejectsSwitchBranchMerge() { + public void createFlowSavesSwitchBranchMergeAsDraft() { LLMDescriptor llmDescriptor = LLMDescriptor.builder() .provider("testProvider") .model("testModel") @@ -1244,12 +1295,16 @@ public class FlowControllerTest { .build()) .build()); - ResponseStatusException exception = assertThrows( - ResponseStatusException.class, - () -> flowController.createFlow(request, new LoginEntity("testuser", "testpassword"))); - - assertEquals(HttpStatus.BAD_REQUEST, exception.getStatusCode()); - assertTrue(exception.getReason().contains("Exclusive routing branches must not merge")); + // Same as the conditional case: a switch-branch merge is not-yet-executable routing, saved + // as a DRAFT and reported by the execution validator rather than rejected at save time. + FlowView created = flowController.createFlow(request, new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(created); + assertEquals(FlowViewStatus.DRAFT, created.status()); + List validation = flowController + .getFlowValidation(created.id(), new LoginEntity("testuser", "testpassword")).getBody(); + assertNotNull(validation); + assertTrue(validation.stream().anyMatch(e -> e.code() == ValidationErrorCode.EXCLUSIVE_BRANCH_MERGE), + () -> "expected EXCLUSIVE_BRANCH_MERGE among execution errors: " + validation); } @Test