kaxil commented on code in PR #71046:
URL: https://github.com/apache/airflow/pull/71046#discussion_r3711216101
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -215,13 +227,17 @@ def execute_complete(self, context: Context,
generated_output: str, event: dict[
# when allow_modifications=False, bypassing the read-only approval
flow.
if getattr(self, "allow_modifications", False) and params_input:
modified = params_input.get("output")
+ if isinstance(modified, list) and all(isinstance(item, str) for
item in modified):
Review Comment:
When the reviewer clears the multi-select, `FieldMultiSelect.handleChange`
sets the param value to `null` (`updatedOptions.length ? ... : null`) and
`getHITLFormData` forwards every param value, so `params_input` arrives as
`{"output": None}`. That falls past both checks here and `execute_complete`
returns `generated_output`, so an empty selection silently re-applies the LLM's
original branches, and `_parse_reviewed_branches`'s "selects no branches" error
is no longer reachable from the form. Worth deciding whether a cleared
selection should be rejected rather than read as "no change".
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_branch.py:
##########
@@ -121,7 +121,9 @@ def execute(self, context: Context) -> str | Iterable[str]
| None:
f"```\nPrompt: {self.prompt}\n\nChosen branch(es):
{chosen}\n```"
)
modification_schema = (
- None if self.allow_multiple_branches else {"type": "string",
"enum": choices}
+ {"type": "array", "enum": choices}
Review Comment:
`{"type": "array", "enum": choices}` is not valid JSON Schema for a pick
list: `enum` at the array level means the whole array must equal one of the
entries, so `["task_a"]` fails validation against `enum: ["task_a", "task_b"]`.
That stays invisible on 3.3+ because nothing validates `params_input` there,
but the `HITLTrigger` fallback in `defer_for_approval` is still live for < 3.3,
and `_handle_response` does `self.params[key] = value`, which runs
`Param.resolve` -> `jsonschema.validate`. Running that with real
`Param`/`ParamsDict`, every submission fails, including an unmodified one:
```
Invalid input for param output: ['task_a'] is not one of ['task_a', 'task_b']
```
so the reviewer's Approve becomes a `HITLTriggerEventError` and the task
fails.
`example_params_ui_tutorial.py` spells out the convention: "If you want to
select multiple items from a fixed list JSON schema does not allow to use enum.
In this case the type `array` is being used together with `examples` as pick
list." `FieldMultiSelect` reads `param.schema.examples ?? param.schema.enum`,
so `examples` renders the same widget and validates. To keep a server-side
check as well:
```python
{"type": "array", "items": {"type": "string", "enum": choices}, "examples":
choices}
```
which renders the multi-select and still rejects a task ID outside the list
on the legacy path. The mixin docstring recommending `{"type": "array", "enum":
[...]}` needs the same fix. The unit tests patch `HITLTrigger` with `autospec`,
so none of them reach the real validation.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -215,13 +227,17 @@ def execute_complete(self, context: Context,
generated_output: str, event: dict[
# when allow_modifications=False, bypassing the read-only approval
flow.
if getattr(self, "allow_modifications", False) and params_input:
modified = params_input.get("output")
+ if isinstance(modified, list) and all(isinstance(item, str) for
item in modified):
+ # Compact so an unchanged selection compares equal to
generated_output
+ modified = json.dumps(modified, separators=(",", ":"))
if modified is not None and not isinstance(modified, str):
# On the awaiting_input path nothing upstream schema-validates
params_input
# (HITLTrigger did on the legacy path), so enforce the string
contract here
# rather than returning a non-string as the task's output.
raise HITLTriggerEventError(
{
- "error": f"Modified output must be a string, got
{type(modified).__name__}.",
+ "error": f"Modified output must be a string or a list
of strings, "
Review Comment:
`type(modified).__name__` is `list` whenever a list reaches this branch, so
a mixed list reads as "Modified output must be a string or a list of strings,
got list." Reporting the offending item's type instead would point at the real
problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
