Merge pull request #1745 from gooddata/snapshot-master-47b32167-to-rel/dev

[bot] Merge master/47b32167 into rel/dev

Author: yenkins-admin

packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py

@@ -119,7 +119,31 @@ 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.
+    """
+    if not emails:
+        return set()
+    try:
+        # RSQL quoted-string escaping: backslash first, then the enclosing quote char,
+        # or an email like o'hara@example.com breaks the filter into invalid RSQL.
+        escaped = [email.replace("\\", "\\\\").replace("'", "\\'") for email in emails]
+        quoted = ",".join(f"'{email}'" for email in escaped)
+        resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=in=({quoted})")
+        return {u.id for u in (resp.data or [])}
+    except Exception:
+        return set()
+
+
+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"))
@@ -134,7 +158,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(
@@ -485,7 +516,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

@@ -5,6 +5,7 @@
 from gooddata_eval.core.agentic.alert_skill import (
     AlertEvaluation,
     _check_filters,
+    _check_recipients,
     _check_trigger,
     _deep_subset,
     _normalize_expected_output,
@@ -141,6 +142,83 @@ def test_normalize_expected_filters_treats_prose_filters_column_as_unspecified()
     assert _check_filters(expected, {"filters": [_ATTR_FILTER]}) is True
 
 
+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=in=('user@example.com')")
+
+
+def test_check_recipients_escapes_apostrophe_in_email_for_rsql_filter():
+    # o'hara@example.com must not break the RSQL filter string -- the apostrophe
+    # has to be escaped before interpolation, same as the query engine requires.
+    expected = _normalize_expected_output({"Recipients": ["o'hara@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=in=('o\\'hara@example.com')"
+    )
+
+
+def test_check_recipients_resolves_multiple_emails_in_a_single_bulk_request():
+    # N expected recipients must cost one request, not N -- confirmed against the
+    # live Users entities API that RSQL `=in=(...)` returns only the matching subset.
+    expected = _normalize_expected_output({"Recipients": ["a@example.com", "b@example.com"]})
+    mock_sdk = MagicMock()
+    mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [
+        MagicMock(id="user.a"),
+        MagicMock(id="user.b"),
+    ]
+    assert _check_recipients(expected, {"internal_recipients": ["user.a", "user.b"]}, sdk=mock_sdk) is True
+    mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(
+        filter="email=in=('a@example.com','b@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,