Skip to content

Commit dec8555

Browse files
committed
Fix output type loss in AgentOperator's human-in-the-loop review
When enable_hitl_review=True and output_type is a non-str, non-BaseModel type (e.g. list[str], bool), the reviewed output was serialized with str(output), producing a Python repr rather than valid JSON. Rehydrating it back into output_type then silently failed and returned the raw repr string instead of the original value. Serialize with TypeAdapter instead, and reuse the existing rehydrate_pydantic_output helper (already used by the require_approval path) instead of a separate, broken json.loads fallback.
1 parent 6f88f29 commit dec8555

4 files changed

Lines changed: 84 additions & 15 deletions

File tree

‎providers/common/ai/src/airflow/providers/common/ai/mixins/hitl_review.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
from datetime import timedelta
2222
from typing import TYPE_CHECKING, Any, Protocol
2323

24-
from pydantic import BaseModel
24+
from pydantic import BaseModel, TypeAdapter
2525

2626
from airflow.providers.common.ai.exceptions import HITLMaxIterationsError
2727
from airflow.providers.common.ai.utils.hitl_review import (
@@ -271,5 +271,5 @@ def _to_string(output: Any) -> str:
271271
if isinstance(output, BaseModel):
272272
return output.model_dump_json()
273273
if not isinstance(output, str):
274-
return str(output)
274+
return TypeAdapter(type(output)).dump_json(output).decode()
275275
return output

‎providers/common/ai/src/airflow/providers/common/ai/operators/agent.py‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,13 @@
1818

1919
from __future__ import annotations
2020

21-
import json
2221
from collections.abc import Sequence
2322
from dataclasses import replace
2423
from datetime import timedelta
2524
from functools import cached_property
2625
from typing import TYPE_CHECKING, Any, ClassVar
2726

28-
from pydantic import BaseModel
27+
from pydantic import BaseModel, TypeAdapter
2928

3029
from airflow.providers.common.ai.hooks.pydantic_ai import PydanticAIHook
3130
from airflow.providers.common.ai.mixins.hitl_review import HITLReviewMixin
@@ -493,16 +492,11 @@ def execute(self, context: Context) -> Any:
493492
output,
494493
message_history=result.all_messages(),
495494
)
496-
if isinstance(self.output_type, type) and issubclass(self.output_type, BaseModel):
497-
return rehydrate_pydantic_output(
498-
self.output_type,
499-
result_str,
500-
serialize_output=self._serialize_model_output,
501-
)
502-
try:
503-
return json.loads(result_str)
504-
except (ValueError, TypeError):
505-
return result_str
495+
return rehydrate_pydantic_output(
496+
self.output_type,
497+
result_str,
498+
serialize_output=self._serialize_model_output,
499+
)
506500

507501
if self._serialize_model_output and isinstance(output, BaseModel):
508502
output = output.model_dump()
@@ -556,4 +550,6 @@ def regenerate_with_feedback(self, *, feedback: str, message_history: Any) -> tu
556550
output = result.output
557551
if isinstance(output, BaseModel):
558552
output = output.model_dump_json()
559-
return str(output), result.all_messages()
553+
elif not isinstance(output, str):
554+
output = TypeAdapter(type(output)).dump_json(output).decode()
555+
return output, result.all_messages()

‎providers/common/ai/tests/unit/common/ai/mixins/test_hitl_review.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,12 @@
2323
if not AIRFLOW_V_3_1_PLUS:
2424
pytest.skip("Human in the loop is only compatible with Airflow >= 3.1.0", allow_module_level=True)
2525

26+
import json
2627
from datetime import timedelta
2728
from unittest.mock import MagicMock, patch
2829

30+
from pydantic import BaseModel
31+
2932
from airflow.providers.common.ai.exceptions import HITLMaxIterationsError
3033
from airflow.providers.common.ai.mixins.hitl_review import HITLReviewMixin
3134
from airflow.providers.common.ai.utils.hitl_review import (
@@ -107,6 +110,30 @@ def context(mock_ti):
107110
return {"task_instance": mock_ti}
108111

109112

113+
class TestToString:
114+
115+
def test_str_output_passes_through_unchanged(self):
116+
assert HITLReviewMixin._to_string("plain text") == "plain text"
117+
118+
def test_base_model_output_uses_model_dump_json(self):
119+
class Summary(BaseModel):
120+
text: str
121+
122+
assert HITLReviewMixin._to_string(Summary(text="hi")) == '{"text":"hi"}'
123+
124+
def test_list_output_is_valid_json_not_python_repr(self):
125+
result = HITLReviewMixin._to_string(["tag-a", "tag-b"])
126+
127+
assert result == '["tag-a","tag-b"]'
128+
assert json.loads(result) == ["tag-a", "tag-b"]
129+
130+
def test_bool_output_is_valid_json_not_python_repr(self):
131+
result = HITLReviewMixin._to_string(True)
132+
133+
assert result == "true"
134+
assert json.loads(result) is True
135+
136+
110137
class TestHITLReviewMixin:
111138
@patch("airflow.providers.common.ai.mixins.hitl_review.time.sleep", autospec=True)
112139
def test_pushes_session_and_output_on_start(

‎providers/common/ai/tests/unit/common/ai/operators/test_agent.py‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,31 @@ def test_execute_with_hitl_rehydrates_base_model(self, mock_hook_cls, mock_run_h
432432
assert result.text == "Approved summary"
433433
assert result.score == 0.9
434434

435+
@pytest.mark.skipif(
436+
not AIRFLOW_V_3_1_PLUS, reason="Human in the loop is only compatible with Airflow >= 3.1.0"
437+
)
438+
@patch("airflow.providers.common.ai.operators.agent.AgentOperator.run_hitl_review", autospec=True)
439+
@patch("airflow.providers.common.ai.operators.agent.PydanticAIHook", autospec=True)
440+
def test_execute_with_hitl_rehydrates_non_base_model_output(self, mock_hook_cls, mock_run_hitl):
441+
mock_result = _make_mock_run_result(["tag-a", "tag-b"])
442+
mock_agent = MagicMock(spec=["run_sync"])
443+
mock_agent.run_sync.return_value = mock_result
444+
mock_hook_cls.get_hook.return_value.create_agent.return_value = mock_agent
445+
mock_run_hitl.return_value = '["tag-a", "tag-b"]'
446+
447+
op = AgentOperator(
448+
task_id="test",
449+
prompt="Summarize",
450+
llm_conn_id="my_llm",
451+
output_type=list[str],
452+
enable_hitl_review=True,
453+
hitl_timeout=timedelta(minutes=5),
454+
)
455+
context = MagicMock()
456+
result = op.execute(context=context)
457+
458+
assert result == ["tag-a", "tag-b"]
459+
435460
@pytest.mark.skipif(
436461
not AIRFLOW_V_3_1_PLUS, reason="Human in the loop is only compatible with Airflow >= 3.1.0"
437462
)
@@ -584,6 +609,27 @@ def test_regenerate_with_feedback_serializes_base_model_output(self, mock_hook_c
584609

585610
assert output == '{"text":"Revised","score":0.0}'
586611

612+
@patch("airflow.providers.common.ai.operators.agent.PydanticAIHook", autospec=True)
613+
def test_regenerate_with_feedback_serializes_non_base_model_output(self, mock_hook_cls):
614+
mock_result = _make_mock_run_result(["tag-a", "tag-b"])
615+
mock_result.all_messages.return_value = []
616+
mock_agent = MagicMock(spec=["run_sync"])
617+
mock_agent.run_sync.return_value = mock_result
618+
mock_hook_cls.get_hook.return_value.create_agent.return_value = mock_agent
619+
620+
op = AgentOperator(
621+
task_id="test",
622+
prompt="test",
623+
llm_conn_id="my_llm",
624+
output_type=list[str],
625+
)
626+
output, _ = op.regenerate_with_feedback(
627+
feedback="Expand",
628+
message_history=[],
629+
)
630+
631+
assert output == '["tag-a","tag-b"]'
632+
587633

588634
class TestAgentOperatorDurable:
589635
def test_durable_param_stored(self):

0 commit comments

Comments
 (0)