fix: null-safe IODescriptor equals/hashCode + normalize HumanDecision option names
Live-testing the assistant (now that the empty-plan fix lets qwen3 produce a
real plan) surfaced a 500 on a plan containing a HumanDecisionBlock. Two
causes, both fixed:
1. IODescriptor.equals/hashCode NPE'd when name was null (name.equals /
name.hashCode). FlowDataValidator.validateBlock compares block outputs via
IODescriptor.equals, so a null-named output made the @ValidFlowStructure
ConstraintValidator throw -> Hibernate HV000028 -> HTTP 500 instead of a
clean validation error. Made both null-safe with java.util.Objects. Now a
malformed config is reported as an invalid flow (and repaired), never a 500 -
a general robustness fix for any flow, not just assistant-generated ones.
2. The null-named outputs came from the model emitting HumanDecision options as
{label, value} instead of {name, label} (name is the routing key / branch
output). buildBlock now normalizes options (normalizeHumanDecisionOptions):
a missing option name is filled from its "value" (the intended routing key)
or a slug of its "label", so every branch gets a real, non-null output name
and the flow is valid.
Tests: draftNormalizesHumanDecisionOptionNamesFromValueOrLabel (options given
as {label,value} and {label} -> names "yes"/"reject", no null outputs, valid).
Full suite green (434).
This commit is contained in:
parent
653184eeae
commit
97ad1776a6
|
|
@ -1152,6 +1152,7 @@ public class FlowAssistantService {
|
|||
normalizedConfig.put("name", defaultIfBlank(draft.name(), defaultIfBlank(blockPlan.purpose(), blockPlan.blockType())));
|
||||
injectSystemManagedFields(normalizedConfig, descriptor, model);
|
||||
ensureRequiredTextDefaults(normalizedConfig, descriptor, blockPlan);
|
||||
normalizeHumanDecisionOptions(normalizedConfig, descriptor);
|
||||
normalizeHttpServerCallAuthorization(normalizedConfig, blockPlan);
|
||||
normalizeMcpAgentSharedMemory(normalizedConfig, blockPlan, model, requireSharedMemorySemantics);
|
||||
ensureSequentialInputPlaceholder(normalizedConfig, descriptor, blockPlan, blockIndex, blockCount);
|
||||
|
|
@ -1340,6 +1341,44 @@ public class FlowAssistantService {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* HumanDecisionOption is (name, label): name is the routing key / branch output, label the
|
||||
* display text. Models routinely emit "value" (or only "label") instead of "name", leaving name
|
||||
* null - which produces null-named branch outputs. Fill a missing option name from its "value"
|
||||
* (the intended routing key) or from a slug of its "label", so the flow is valid and each branch
|
||||
* has a real output name.
|
||||
*/
|
||||
private void normalizeHumanDecisionOptions(ObjectNode config,
|
||||
BlockCatalogService.AssistantPromptBlockDescriptor descriptor) {
|
||||
if (!isConfigurationType(descriptor.configurationType(), "HumanDecisionBlockConfiguration")) {
|
||||
return;
|
||||
}
|
||||
if (!(config.get("options") instanceof ArrayNode options)) {
|
||||
return;
|
||||
}
|
||||
for (JsonNode option : options) {
|
||||
if (!(option instanceof ObjectNode optionObject) || hasTextValue(optionObject.get("name"))) {
|
||||
continue;
|
||||
}
|
||||
String derived = hasTextValue(optionObject.get("value"))
|
||||
? textOrNull(optionObject.get("value")).trim()
|
||||
: hasTextValue(optionObject.get("label"))
|
||||
? slugifyOptionName(textOrNull(optionObject.get("label")))
|
||||
: null;
|
||||
if (derived != null && !derived.isBlank()) {
|
||||
optionObject.put("name", derived);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private String slugifyOptionName(String label) {
|
||||
String slug = label.trim().toLowerCase(Locale.ROOT).replaceAll("[^a-z0-9]+", "-").replaceAll("(^-+|-+$)", "");
|
||||
if (slug.isBlank()) {
|
||||
return "option";
|
||||
}
|
||||
return Character.isLetter(slug.charAt(0)) ? slug : "opt-" + slug;
|
||||
}
|
||||
|
||||
private void removeSystemManagedFields(ObjectNode config) {
|
||||
config.remove(List.of("provider", "model", "llmDescriptor", "ids", "inputs", "outputs", "skills"));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -81,7 +81,7 @@ public class IODescriptor {
|
|||
|
||||
@Override
|
||||
public int hashCode() {
|
||||
return name.hashCode();
|
||||
return java.util.Objects.hashCode(name);
|
||||
}
|
||||
|
||||
@Override
|
||||
|
|
@ -91,6 +91,6 @@ public class IODescriptor {
|
|||
if (obj == null || getClass() != obj.getClass())
|
||||
return false;
|
||||
IODescriptor other = (IODescriptor) obj;
|
||||
return name.equals(other.name);
|
||||
return java.util.Objects.equals(name, other.name);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2759,6 +2759,51 @@ public class AssistantControllerTest {
|
|||
assertEquals("Help the user plan a trip", configuration.getGoalDescription());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void draftNormalizesHumanDecisionOptionNamesFromValueOrLabel() {
|
||||
// Models routinely emit HumanDecision options as {label, value} instead of {name, label},
|
||||
// leaving the routing key (name) null - which used to make null-named branch outputs and a
|
||||
// 500. The backend now fills name from value (or a slug of label) so the flow is valid.
|
||||
Answer<String> answer = invocation -> {
|
||||
String prompt = invocation.getArgument(1, String.class);
|
||||
if (prompt.contains("TASK: PLAN")) {
|
||||
return TestAssistantResponses.wrap(java.util.Map.of("rationale", "One review decision.",
|
||||
"plan", java.util.Map.of("name", "Review", "description", "desc",
|
||||
"blocks", java.util.List.of(java.util.Map.of("blockId", "b1",
|
||||
"blockType", "HumanDecisionBlock", "purpose", "Approve or reject")))));
|
||||
}
|
||||
if (prompt.contains("TASK: BLOCK_CONFIG")) {
|
||||
return TestAssistantResponses.wrap(java.util.Map.of("rationale", "decision config",
|
||||
"block", java.util.Map.of("blockId", "b1", "name", "review",
|
||||
"config", java.util.Map.of(
|
||||
"question", "Is it ready for approval?",
|
||||
"options", java.util.List.of(
|
||||
java.util.Map.of("label", "Approve", "value", "yes"),
|
||||
java.util.Map.of("label", "Reject")), // only label, no value
|
||||
"rationaleRequired", true))));
|
||||
}
|
||||
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("a human review that approves or rejects", MODEL, 1));
|
||||
|
||||
assertNotNull(response);
|
||||
assertTrue(response.valid(), () -> "Unexpected validation errors: " + response.validationErrors());
|
||||
it.cnr.isti.workflow.manager.blocks.configurations.HumanDecisionBlockConfiguration configuration =
|
||||
(it.cnr.isti.workflow.manager.blocks.configurations.HumanDecisionBlockConfiguration)
|
||||
response.flow().flow().getBlocks().getFirst().getSpecificConfiguration();
|
||||
java.util.List<String> optionNames = configuration.getOptions().stream()
|
||||
.map(it.cnr.isti.workflow.manager.blocks.configurations.HumanDecisionOption::name).toList();
|
||||
// "yes" taken from value; "reject" slugged from the label (no value given).
|
||||
assertEquals(java.util.List.of("yes", "reject"), optionNames);
|
||||
// The branch outputs are named accordingly (never null).
|
||||
assertTrue(response.flow().flow().getBlocks().getFirst().getOutputs().stream()
|
||||
.allMatch(output -> output.getName() != null && !output.getName().isBlank()));
|
||||
}
|
||||
|
||||
static class TestAssistantResponses {
|
||||
private static final String PROVIDER = "InternalOllama";
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue