fix(gooddata-eval): check internal_recipients in alert recipients comparison

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,

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,