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 1a7d2a188..b1972e9dd 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 @@ -109,7 +109,28 @@ def _check_metric(expected: CatalogMetricAlert, actual_args: dict) -> bool: return expected.metric_id == act_metric -def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: +def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[str]: + """Best-effort map of expected recipient emails to internal GoodData user ids. + + Some notification channels are workspace-restricted to internal users -- + `create_metric_alert` then addresses the alert by internal user id + (`internal_recipients`), never by email, so an expected email has to be + resolved before it can be compared against that field. Failures (no + matching user, no permission, network error) are swallowed: the caller + treats an empty result the same as "this delivery path doesn't match", + which is correct -- it doesn't mean the alert itself failed. + """ + ids: set[str] = set() + for email in emails: + try: + resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=='{email}'") + ids.update(u.id for u in (resp.data or [])) + except Exception: + pass + return ids + + +def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None) -> bool: if not expected.recipients: return True act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) @@ -124,7 +145,14 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: act_recip = act_recip_raw else: act_recip = [] - return set(expected.recipients) == set(act_recip or []) + if set(expected.recipients) == set(act_recip or []): + return True + act_internal = actual_args.get("internal_recipients") + if sdk is not None and isinstance(act_internal, list) and act_internal: + internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected.recipients) + if internal_recipient_ids & set(act_internal): + return True + return False def generate_simulated_alert_response( @@ -388,7 +416,7 @@ def _run_once(conv_id: str) -> AlertRunResult: trigger_correct=tool_called and _check_trigger(expected, actual_args), filters_correct=tool_called and _check_filters(expected, actual_args), metric_correct=tool_called and _check_metric(expected, actual_args), - recipients_correct=tool_called and _check_recipients(expected, actual_args), + recipients_correct=tool_called and _check_recipients(expected, actual_args, sdk=sdk), ) return AlertRunResult( conversation_id=conv_id, diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index fa5dcbedd..17e77e450 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -4,6 +4,7 @@ from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, + _check_recipients, _check_trigger, _deep_subset, _normalize_expected_output, @@ -64,6 +65,54 @@ def test_check_trigger_once_needs_explicit_once(): assert _check_trigger(expected, {"trigger": "ONCE_PER_INTERVAL"}) is False # real model error stays a fail +def test_check_recipients_matches_external_recipients_without_sdk(): + # The common path never needs a network call at all -- confirms adding the + # internal_recipients fallback doesn't force a lookup when it isn't needed. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + assert _check_recipients(expected, {"recipients": ["user@example.com"]}, sdk=mock_sdk) is True + mock_sdk._client.entities_api.get_all_entities_users.assert_not_called() + + +def test_check_recipients_matches_internal_recipients_via_resolved_user_id(): + # Some notification channels are workspace-restricted to internal users -- + # create_metric_alert then addresses the alert by internal user id via + # `internal_recipients`, never by email, so the plain email/external-recipients + # comparison alone can never match this delivery path. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ + MagicMock(id="user.abc123"), + ] + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is True + mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(filter="email=='user@example.com'") + + +def test_check_recipients_internal_recipients_mismatch_still_fails(): + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ + MagicMock(id="someone.else"), + ] + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False + + +def test_check_recipients_internal_recipients_without_sdk_fails_gracefully(): + # No sdk available to resolve the email -> no crash, just no match (the plain + # external-recipients comparison already ran and failed by this point). + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=None) is False + + +def test_check_recipients_resolution_failure_fails_gracefully(): + # A lookup error (permissions, network) must not crash the evaluation -- + # it just means this comparison path can't match, same as no sdk at all. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.side_effect = RuntimeError("boom") + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False + + def test_alert_evaluation_strict_pass(): ev = AlertEvaluation( alert_created=True,