Skip to content

Support assigned reviewers in LLM approval reviews - #72157

Merged
guan404ming merged 3 commits into
apache:mainfrom
guan404ming:llm-approval-assigned-users
Sep 18, 2026
Merged

guan404ming merged 3 commits into
apache:mainfrom
guan404ming:llm-approval-assigned-users

Conversation

@guan404ming

@guan404ming guan404ming commented Aug 27, 2026 •

Copy link
Copy Markdown
Member

Why

  • An LLM approval review was open to every user with the HITL permission.
  • There was no way to route a decision to the people who actually own it.

How

  • Add approval_assigned_users to name the reviewers allowed to answer.
  • Forward those users to the review request, as HITLOperator already does.
  • Validate the entry shape at parse time and reject the parameter on cores older than Airflow 3.1.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

@guan404ming
guan404ming force-pushed the llm-approval-assigned-users branch from b63e1b0 to 8e8b27c Compare August 27, 2026 15:10
@guan404ming guan404ming changed the title Restrict LLM approval reviews to assigned users Support assigned reviewers in LLM approval reviews Aug 27, 2026
@guan404ming
guan404ming force-pushed the llm-approval-assigned-users branch from 8e8b27c to 3030164 Compare August 27, 2026 15:35
@guan404ming
guan404ming marked this pull request as ready for review August 28, 2026 02:13
@guan404ming
guan404ming force-pushed the llm-approval-assigned-users branch from 3030164 to db4da4a Compare September 9, 2026 14:06
@guan404ming
guan404ming force-pushed the llm-approval-assigned-users branch from db4da4a to 13467ee Compare September 16, 2026 14:53
Comment thread providers/common/ai/tests/unit/common/ai/operators/test_llm.py Outdated
Comment thread providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py Outdated
Comment thread providers/common/ai/src/airflow/providers/common/ai/mixins/approval.py Outdated
Comment thread providers/common/ai/src/airflow/providers/common/ai/operators/llm.py Outdated
Comment thread providers/common/ai/docs/operators/llm.rst Outdated
Comment thread providers/common/ai/tests/unit/common/ai/mixins/test_approval.py Outdated
Comment thread providers/common/ai/src/airflow/providers/common/ai/operators/llm_sql.py Outdated
The review was always open to every user with the HITL permission, so an
operator could not route an approval to the people who actually own the
decision. The Task SDK already accepts assigned users for a review; the
LLM operators simply never passed any.
… rules

A malformed approval_assigned_users entry only failed inside defer_for_approval, after the model call was billed, and the version guard tested the raw argument while the mixin tested the normalized one. The old-core guard test never ran on any lane because its skipif contradicted the class-level one. The docs did not say that id is the auth manager user id, that 3.1.0 to 3.1.5 compare name as well, or that the list is fixed once the review row exists.
@guan404ming
guan404ming force-pushed the llm-approval-assigned-users branch from 9f16fdd to 65c6efd Compare September 17, 2026 13:52
@guan404ming
guan404ming requested a review from kaxil September 17, 2026 15:54
@kaxil

kaxil commented Sep 17, 2026

Copy link
Copy Markdown
Member

Round 1 is all addressed at 65c6efd, and I re-derived each thread against the code rather than going off the replies: assigned_users is passed unconditionally now, the 3.1.6 comparison boundary in the docs matches what hitl.py does at 3.1.5 versus 3.1.6, the stale-list caveat is still accurate against this branch's base, and the old-core test sits inside the old-core class so the sub-3.1 compat lane actually runs it.

One non-blocking doc thought, nothing gating:

llm.rst L283-285 gives FAB as the concrete answer for id, and FAB is not the default auth manager. Since a wrong id locks out every reviewer on 3.1.0 through 3.1.5, as the next sentence says, it would help to name the default too: [core] auth_manager defaults to SimpleAuthManager, whose get_id() returns the username, so there id is the username from simple_auth_manager_users.

@guan404ming

Copy link
Copy Markdown
Member Author

Added in 6e25e2f: with the default SimpleAuthManager, id is the username from simple_auth_manager_users.

@guan404ming
guan404ming merged commit 4274192 into apache:main Sep 18, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants