diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index 015524453..d1a6e22f5 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -125,6 +125,29 @@ def _parse_recipients(recipients_str: str | None) -> list[str] | None: return [r.strip() for r in recipients_str.replace(";", ",").split(",") if r.strip()] +def _recipient_list(value: object, *, split_bare: bool) -> list[str]: + """A recipient argument as a list, decoded the way gen-ai does before calling the server. + + The tool schema declares both recipient fields ``anyOf: [array of string, string, null]``, + and some models send the list JSON-encoded as a string (``'["admin"]'``). Empty or + ``"null"`` means none; any other string that is not a JSON list of strings is a bare + value -- one user id, or with ``split_bare`` a comma/semicolon-separated email list + (gen-ai forwards a bare string as one entry; the split is this checker's own leniency). + A non-string list entry is kept as its ``str()`` so it compares unequal instead of raising. + """ + if isinstance(value, list): + return [str(item) for item in value] + if not isinstance(value, str) or value.strip() in ("", "null"): + return [] + try: + parsed = json.loads(value) + except ValueError: + parsed = None + if isinstance(parsed, list) and all(isinstance(item, str) for item in parsed): + return parsed + return (_parse_recipients(value) or []) if split_bare else [value] + + def _deep_subset(expected: object, actual: object) -> bool: """Return True if expected is a recursive subset of actual.""" if isinstance(expected, dict) and isinstance(actual, dict): @@ -396,35 +419,14 @@ def _check_recipients( ) -> bool: if not expected.recipients: return True - act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) - if isinstance(act_recip_raw, str): - # external_recipients is JSON-encoded (e.g. '["email@example.com"]') - try: - parsed = json.loads(act_recip_raw) - act_recip = parsed if isinstance(parsed, list) else _parse_recipients(act_recip_raw) - except (json.JSONDecodeError, ValueError): - act_recip = _parse_recipients(act_recip_raw) - elif isinstance(act_recip_raw, list): - act_recip = act_recip_raw - else: - act_recip = [] - act_internal_raw = actual_args.get("internal_recipients") - # internal_recipients is declared `anyOf: [array of string, string, null]` in the - # create_metric_alert tool schema -- a single id as a bare string is schema-legal, - # not a malformed call, so it needs the same string/list normalization already - # applied to recipients/external_recipients above. - if isinstance(act_internal_raw, str): - act_internal = [act_internal_raw] - elif isinstance(act_internal_raw, list): - act_internal = act_internal_raw - else: - act_internal = [] + act_recip = _recipient_list(actual_args.get("recipients", actual_args.get("external_recipients")), split_bare=True) + act_internal = _recipient_list(actual_args.get("internal_recipients"), split_bare=False) wants_current_user, expected_emails = _split_current_user(expected.recipients) if wants_current_user: if current_user_id is None or current_user_id not in act_internal: return False other_internal = set(act_internal) - {current_user_id} - external = set(act_recip or []) + external = set(act_recip) if external - set(expected_emails): return False # An email not sent externally must arrive as its user's id (workspace-restricted channel). @@ -436,7 +438,7 @@ def _check_recipients( # One lookup per email: a bulk lookup returns only the users that exist, hiding a missing one. ids_per_email = [_resolve_internal_recipient_ids(sdk, [email]) for email in unmatched] return all(ids_per_email) and other_internal == set().union(*ids_per_email) - if set(expected_emails) == set(act_recip or []): + if set(expected_emails) == set(act_recip): return True if sdk is not None and act_internal: internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected_emails) diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index 3cf4dd79d..b0ff5481d 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -356,6 +356,64 @@ def test_check_recipients_accepts_the_current_user_alongside_an_email() -> None: assert _check_recipients(expected, extra, current_user_id="admin") is False +@pytest.mark.parametrize( + ("internal_recipients", "passes"), + [ + # Some models (haiku) JSON-encode the list; gen-ai decodes it before calling the server. + ('["admin"]', True), + ("admin", True), + (["admin"], True), + ('["someone_else"]', False), + ("[]", False), + ("null", False), + ("", False), + (" ", False), + # Not a list of strings, so the whole string is one (unknown) id. + ('["admin", 1]', False), + ], +) +def test_check_recipients_decodes_a_json_encoded_internal_recipients_string( + internal_recipients: object, passes: bool +) -> None: + expected = _normalize_expected_output({"Recipient(s)": "me"}) + args = {"internal_recipients": internal_recipients} + assert _check_recipients(expected, args, sdk=MagicMock(), current_user_id="admin") is passes + + +def test_check_recipients_decodes_json_encoded_strings_next_to_an_email() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me, bob@example.com"}) + args = {"internal_recipients": '["admin"]', "external_recipients": '["bob@example.com"]'} + assert _check_recipients(expected, args, current_user_id="admin") is True + extra = {"internal_recipients": '["admin", "carol.id"]', "external_recipients": '["bob@example.com"]'} + assert _check_recipients(expected, extra, current_user_id="admin") is False + + +@pytest.mark.parametrize( + ("external_recipients", "passes"), + [ + ('["a@example.com", "b@example.com"]', True), + (["a@example.com", "b@example.com"], True), + ("a@example.com; b@example.com", True), + ("a@example.com, b@example.com", True), + ('["a@example.com"]', False), + ("null", False), + ("", False), + (" ; ", False), + (None, False), + ], +) +def test_check_recipients_normalizes_every_external_recipients_shape(external_recipients: object, passes: bool) -> None: + expected = _normalize_expected_output({"Recipient(s)": "a@example.com, b@example.com"}) + args = {"external_recipients": external_recipients} + assert _check_recipients(expected, args) is passes + + +def test_check_recipients_scores_a_non_string_internal_recipient_false_instead_of_raising() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me"}) + args = {"internal_recipients": ["admin", {"id": "carol.id"}]} + assert _check_recipients(expected, args, current_user_id="admin") is False + + def test_check_recipients_accepts_an_email_delivered_as_a_user_id_next_to_the_current_user() -> None: expected = _normalize_expected_output({"Recipient(s)": "me, bob@example.com"}) mock_sdk = MagicMock() @@ -411,7 +469,12 @@ def test_sim_user_still_volunteers_an_email_recipient() -> None: assert "Proactively include your email recipient" in prompt -def test_run_agentic_alert_skill_scores_a_current_user_alert() -> None: +@pytest.mark.parametrize( + "internal_recipients", + # As sent in the tool call: a JSON list, or that list JSON-encoded into a string. + ['["admin"]', '"[\\"admin\\"]"'], +) +def test_run_agentic_alert_skill_scores_a_current_user_alert(internal_recipients: str) -> None: asked_turn = ChatResult.model_validate({"text_response": "Which dashboard?", "tool_call_events": []}) created_turn = ChatResult.model_validate( { @@ -419,7 +482,9 @@ def test_run_agentic_alert_skill_scores_a_current_user_alert() -> None: "tool_call_events": [ { "functionName": "create_metric_alert", - "functionArguments": '{"operator": "LESS_THAN", "threshold": 30000, "internal_recipients": ["admin"]}', + "functionArguments": ( + f'{{"operator": "LESS_THAN", "threshold": 30000, "internal_recipients": {internal_recipients}}}' + ), "result": '{"id": "alert-1"}', } ],