kaxil commented on code in PR #72157:
URL: https://github.com/apache/airflow/pull/72157#discussion_r4032118101
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -174,6 +188,7 @@ def defer_for_approval(
defaults=timeout_defaults,
multiple=False,
params=hitl_params,
+ **assignee_kwargs,
Review Comment:
Worth a line in the docs: the execution API writes `assignees` only on the
INSERT branch
([routes/hitl.py#L72-L91](https://github.com/apache/airflow/blob/386132ef49f474add5509cf206cfed936a4645bf/airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py#L72-L91)),
and clearing a task keeps its `ti_id`, so a re-run finds the row already there
and a changed list never lands. That is pre-existing for `subject`, `body` and
`defaults`, but this is the first field where the stale value is an
authorization control: drop someone from `approval_assigned_users`, clear the
task, and they can still approve, while the reviewer you added cannot. Saying
in llm.rst that the list is fixed at first run would cover it.
##########
providers/common/ai/tests/unit/common/ai/operators/test_llm.py:
##########
@@ -359,6 +363,26 @@ def
test_on_approval_timeout_without_prerequisites_raises(self, kwargs):
):
LLMOperator(task_id="t", prompt="p", llm_conn_id="c",
on_approval_timeout="approve", **kwargs)
+ @pytest.mark.skipif(not AIRFLOW_V_3_1_PLUS, reason="assigned_users needs
Airflow 3.1+")
+ @pytest.mark.parametrize(
+ "assigned_users",
+ [{"id": "u1", "name": "alice"}, [{"id": "u1", "name": "alice"}]],
+ ids=["single", "list"],
+ )
+ def test_approval_assigned_users_normalized_to_list(self, assigned_users):
+ op = LLMOperator(task_id="t", prompt="p", llm_conn_id="c",
approval_assigned_users=assigned_users)
+ assert op.approval_assigned_users == [{"id": "u1", "name": "alice"}]
+
+ @pytest.mark.skipif(AIRFLOW_V_3_1_PLUS, reason="guard only fires on cores
before 3.1")
Review Comment:
This `skipif` and the class-level one on line 318 are mutually exclusive, so
the test never runs anywhere: on 3.1+ this marker fires, and below 3.1 the
class marker does. pytest skips an item when any inherited `skipif` condition
is true. There is a real sub-3.1 lane that would otherwise exercise it, the
`3.0.6` row of `PROVIDERS_COMPATIBILITY_TESTS_MATRIX` in
`dev/breeze/src/airflow_breeze/global_constants.py`, which runs unit tests with
common.ai installed. `test_agent.py` covers the equivalent `enable_hitl_review`
guard with
`@patch("airflow.providers.common.ai.operators.agent.AIRFLOW_V_3_1_PLUS",
False)` instead, which runs on every core.
##########
providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py:
##########
@@ -166,6 +174,12 @@ def defer_for_approval(
},
}
+ # Only pass assigned_users when set: cores before 3.2 have no such
argument, and the
Review Comment:
The version here is off, and the conditional below it is unreachable either
way. `assigned_users` has been a parameter of `upsert_hitl_detail` since 3.1.0,
not 3.2 (`git show 3.1.0:task-sdk/src/airflow/sdk/execution_time/hitl.py`); the
PR description carries the same 3.2. Beyond the number, `defer_for_approval`
imports `airflow.providers.standard.triggers.hitl` and
`airflow.sdk.execution_time.hitl` a few lines above, and both fail below 3.1
(the trigger module raises at import, the SDK module does not exist at 3.0.x),
so by the time this line runs the argument is always present. Passing `None` is
also indistinguishable from omitting it, since the callee does `if
assigned_users else []`. `HITLOperator` passes it unconditionally.
##########
providers/common/ai/docs/operators/llm.rst:
##########
@@ -254,6 +254,12 @@ Reject the primary button:
:start-after: [START howto_operator_llm_approval]
:end-before: [END howto_operator_llm_approval]
+By default any user with the permission can answer the review. Pass
+``approval_assigned_users=[{"id": "<user-id>", "name": "<user-name>"}]`` to
+restrict it to named reviewers, the way
+:class:`~airflow.providers.standard.operators.hitl.HITLOperator` does with
+``assigned_users``. This needs Airflow 3.1+.
Review Comment:
"Airflow 3.1+" is right for storage, but the enforcement semantics change
inside that range. On 3.1.0 through 3.1.5 the response route builds
`HITLUser(id=user_id, name=user.get_name())` and tests `hitl_user not in
assigned_users`, which is dict equality over both keys, so a `name` that does
not match what the auth manager reports rejects the assigned reviewer, and
everyone else with it because the list is non-empty. With the default
`approval_timeout=None` the task then waits indefinitely. Id-only matching
arrives in 3.1.6.
While you are here, it would help to say what `id` is: the auth manager's
user id, which under FAB is `str(ab_user.id)`, the numeric row id rather than
the username. The `<user-name>` placeholder reads like it wants a display name.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm.py:
##########
@@ -173,10 +179,17 @@ def __init__(
"a positive approval_timeout to fire. "
"Set both, or leave on_approval_timeout as 'fail'."
)
+ if approval_assigned_users and not AIRFLOW_V_3_1_PLUS:
+ raise
AirflowOptionalProviderFeatureException("approval_assigned_users needs Airflow
3.1+.")
self.require_approval = require_approval
self.approval_timeout = approval_timeout
self.on_approval_timeout = on_approval_timeout
self.allow_modifications = allow_modifications
+ self.approval_assigned_users = (
Review Comment:
Nothing validates the entry shape, so `[{"id": "u1"}]` reaches
`APIHITLUser(id=user["id"], name=user["name"])` as a bare `KeyError: 'name'`,
and a plain `"alice"` is not a dict, so it stays unwrapped and iterates
character by character into `TypeError: string indices must be integers`.
`HITLOperator` has the same gap, but it calls the upsert as its first
statement, whereas `execute` here runs `agent.run_sync()` first, so the failure
lands after the model call is billed and repeats on every retry. A shape check
beside the `on_approval_timeout` validation above would move it to parse time.
Separately, the guard on line 182 tests the pre-normalization argument while
the mixin tests the normalized attribute, so the two disagree on `{}`: it is
falsy here and skips the version guard, then normalizes to `[{}]`, which is
truthy in the mixin and reaches the upsert.
##########
providers/common/ai/tests/unit/common/ai/mixins/test_approval.py:
##########
@@ -176,6 +178,23 @@ def test_array_schema_passes_list_param_value(
defer_kwargs = approval_op_with_modifications.defer.call_args[1]
assert defer_kwargs["kwargs"]["generated_output"] == '["task_a"]'
+ @patch(HITL_TRIGGER_PATH, autospec=True)
+ @patch(UPSERT_HITL_PATH)
+ def test_assigned_users_are_forwarded(self, mock_upsert, mock_trigger_cls,
context):
+ users = [{"id": "u1", "name": "alice"}]
+ op = FakeOperator(approval_assigned_users=users)
+
+ op.defer_for_approval(context, "output")
+
+ assert mock_upsert.call_args[1]["assigned_users"] == users
+
+ @patch(HITL_TRIGGER_PATH, autospec=True)
+ @patch(UPSERT_HITL_PATH)
+ def test_assigned_users_omitted_when_unset(self, mock_upsert,
mock_trigger_cls, approval_op, context):
+ approval_op.defer_for_approval(context, "output")
+
+ assert "assigned_users" not in mock_upsert.call_args[1]
Review Comment:
This assertion passes on the parent commit as well, since the pre-PR code
never passed the kwarg at all, so it does not guard the new behaviour. Its one
live effect is to pin the conditional above: passing
`assigned_users=self.approval_assigned_users` unconditionally is behaviourally
identical on every core that can reach it, but would fail here. `assert
mock_upsert.call_args[1]["assigned_users"] is None` would assert the actual
contract instead.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/llm_sql.py:
##########
@@ -85,7 +85,7 @@ class LLMSQLQueryOperator(LLMOperator):
Human-in-the-Loop approval parameters are inherited from
:class:`~airflow.providers.common.ai.operators.llm.LLMOperator`
(``require_approval``, ``approval_timeout``, ``on_approval_timeout``,
- ``allow_modifications``).
+ ``allow_modifications``, ``approval_assigned_users``).
Review Comment:
`llm_sql.rst` enumerates the inherited approval knobs in prose too,
`allow_modifications` on line 143 and `approval_timeout` and
`on_approval_timeout` on 145, and did not get `approval_assigned_users`. It is
the only one of the five operator pages that was missed.
--
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]
