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 5e0f83f77..b7b771297 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 @@ -9,6 +9,7 @@ from dataclasses import dataclass, field from typing import Any +import httpx from gooddata_sdk import GoodDataSdk from gooddata_eval.core.agentic._catalog import AnomalyDetectionGranularity, CatalogMetricAlert @@ -54,6 +55,11 @@ _TRIGGER_DISPLAY_TO_API = {"Every time": "ALWAYS", "One time": "ONCE"} _ALWAYS_TRIGGER_VALUES = {"Every time", "ALWAYS", "not specified"} +_RELATIVE_OPERATORS = ("INCREASES_BY", "DECREASES_BY", "CHANGES_BY") + +# Expected-recipient spellings that mean the owner of the API token. +_CURRENT_USER_RECIPIENT = "me" +_CURRENT_USER_MARKERS = {"me", "myself", "current user", "current_user"} _TRIGGER_INSTRUCTIONS = { "ALWAYS": ( @@ -242,9 +248,9 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[ `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. + matching user, no permission, network error) are logged and return an + empty set: the caller treats that 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() @@ -254,12 +260,48 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[ 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: + ids = {u.id for u in (resp.data or [])} + except Exception as exc: + print(f"[RECIPIENTS] Looking up users by {len(emails)} recipient email(s) failed: {exc}") return set() + if not ids: + print( + f"[RECIPIENTS] No user has any of the {len(emails)} recipient email(s); internal recipients cannot match." + ) + return ids + + +def _resolve_current_user_id(host: str, token: str) -> str | None: + """Id of the user that owns ``token``, or None if the profile cannot be read. + + The current user is matched by id, not email: that user may have no email at all. + """ + try: + resp = httpx.get( + f"{host.rstrip('/')}/api/v1/profile", + headers={"Authorization": f"Bearer {token}"}, + timeout=30.0, + ) + resp.raise_for_status() + user_id = resp.json().get("userId") + except Exception as exc: + print(f"[RECIPIENTS] Resolving the current user failed: {exc}") + return None + return user_id if isinstance(user_id, str) and user_id else None + +def _split_current_user(recipients: list[str]) -> tuple[bool, list[str]]: + """Whether ``recipients`` names the current user, and the email recipients besides it.""" + emails = [r for r in recipients if r != _CURRENT_USER_RECIPIENT] + return len(emails) != len(recipients), emails -def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None) -> bool: + +def _check_recipients( + expected: CatalogMetricAlert, + actual_args: dict, + sdk: GoodDataSdk | None = None, + current_user_id: str | None = None, +) -> bool: if not expected.recipients: return True act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) @@ -274,8 +316,6 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good act_recip = act_recip_raw else: act_recip = [] - if set(expected.recipients) == set(act_recip or []): - return True 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, @@ -287,8 +327,27 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good act_internal = act_internal_raw else: act_internal = [] + 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 []) + if external - set(expected_emails): + return False + # An email not sent externally must arrive as its user's id (workspace-restricted channel). + unmatched = [e for e in expected_emails if e not in external] + if not unmatched: + return not other_internal + if sdk is None or not other_internal: + return False + # 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 []): + return True if sdk is not None and act_internal: - internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected.recipients) + internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected_emails) if internal_recipient_ids & set(act_internal): return True return False @@ -299,6 +358,7 @@ def generate_simulated_alert_response( expected: CatalogMetricAlert, conversation_history: list, question: str = "", + current_user_id: str | None = None, ) -> str: """Stateful sim-user reply for alert-skill conversation (gpt-4o). @@ -306,6 +366,8 @@ def generate_simulated_alert_response( history — the opening question went straight to the agent, never to the sim-user — so without it rule 5's "the filters your original request implies" refers to text the model cannot see. Optional (defaults to "") to keep the signature backwards compatible. + + ``current_user_id`` lets the sim-user recognise itself as the recipient of a "me" alert. """ if _OpenAI is None: raise RuntimeError( @@ -333,7 +395,30 @@ def generate_simulated_alert_response( threshold = expected.threshold else: threshold = "not specified" - recipients = ", ".join(expected.recipients) if expected.recipients else "not specified" + wants_current_user, emails = _split_current_user(expected.recipients) + recipient_names = list(emails) + if wants_current_user: + user_id_note = f", user id {current_user_id}" if current_user_id else "" + recipient_names.insert(0, f"me (yourself, the current user{user_id_note})") + recipients = ", ".join(recipient_names) if recipient_names else "not specified" + if wants_current_user and not emails: + # An email given here would make the agent send the alert to it instead of the current user. + recipients_rule = ( + "4. The alert is for you yourself. Never give an email address and never ask for one to " + "be added. A proposal whose recipient is you — shown as 'You', the current user or your " + "own user id — has the correct recipient.\n" + ) + elif wants_current_user: + recipients_rule = ( + f"4. Proactively state the email recipients in your first reply: {', '.join(emails)}, " + "and say the alert should also go to you yourself. A recipient shown as 'You', the " + "current user or your own user id is you.\n" + ) + else: + recipients_rule = ( + "4. Proactively include your email recipient in your first reply. " + " Do not wait for the agent to ask — state it alongside the metric and condition answers.\n" + ) trigger = expected.trigger filters = expected.filters @@ -378,14 +463,25 @@ def generate_simulated_alert_response( # and scoring an item against an interval it never asked for is the defect this rule # exists to undo. granularity = (expected.granularity or AnomalyDetectionGranularity.DAY).value - anomaly_rule = ( + period_rule = ( "7. This is an ANOMALY alert. Anomaly detection REQUIRES a time granularity, and that " f"granularity is NOT a date filter. State it in your first reply and repeat it whenever " f"asked: use {granularity} granularity. Rule 5 constrains filters on the metric only — it " "never applies to this detection interval, so never refuse to give one.\n" ) + elif operator in _RELATIVE_OPERATORS: + # The item names no comparison date or period; delegating is an answer the agent accepts, + # refusing loops until the turn budget runs out. + period_rule = ( + "7. This is a comparison alert: keep the comparison your original request describes. The " + "agent may need a comparison date and a period length (day, week, month, quarter or " + "year), and those are NOT a date filter. Rule 5 constrains filters on the metric only — " + "it never applies to the comparison. If the agent asks which date or which period the " + "comparison should use, do not refuse and do not name one yourself: hand the choice back, " + "for example 'Pick whichever date and period fit best.'\n" + ) else: - anomaly_rule = ( + period_rule = ( "7. Do not invent an evaluation period, a granularity or an 'evaluate each run on a X " "basis' instruction your goal never asked for.\n" ) @@ -409,11 +505,10 @@ def generate_simulated_alert_response( " when every one of those fields matches your goal.\n" " A field your goal reports as 'not specified' is one you have NO expectation about: " " accept whatever the agent chose for it and never ask for it to be removed.\n" - "4. Proactively include your email recipient in your first reply. " - " Do not wait for the agent to ask — state it alongside the metric and condition answers.\n" + + recipients_rule + filters_rule + f"6. Proactively state how often you want to be alerted in your first reply: {trigger_request}. " - " Repeat it if the agent proposes a different cadence.\n" + anomaly_rule + "Reply concisely and directly." + " Repeat it if the agent proposes a different cadence.\n" + period_rule + "Reply concisely and directly." ) messages: list = [{"role": "system", "content": system_prompt}] @@ -590,6 +685,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: recipients = [r.strip() for r in raw_recip.replace(";", ",").split(",") if r.strip()] else: recipients = list(raw_recip) + recipients = [_CURRENT_USER_RECIPIENT if str(r).strip().lower() in _CURRENT_USER_MARKERS else r for r in recipients] filters = _normalize_expected_filters(expected) attributes = _normalize_expected_attributes(expected) @@ -665,6 +761,12 @@ def run_agentic_alert_skill( ) -> AgenticAlertSummary: """Run the alert-skill agentic evaluation K times and return a summary.""" expected = _normalize_expected_output(expected_output) + current_user_id: str | None = None + if _CURRENT_USER_RECIPIENT in expected.recipients: + current_user_id = _resolve_current_user_id(host, token) + if current_user_id is None: + # A harness fault: report it as an item error, not as the agent's recipient mistake. + raise RuntimeError("Cannot score a current-user alert: the token owner's user id could not be resolved") run_results: list[AlertRunResult] = [] client = ChatClient( host=host, @@ -761,7 +863,11 @@ def _run_once(conv_id: str) -> AlertRunResult: # exit reason to classify the run as an error rather than an agent failure. try: follow_up = generate_simulated_alert_response( - response_text, expected, conversation_history, question=question + response_text, + expected, + conversation_history, + question=question, + current_user_id=current_user_id, ) except Exception as exc: # noqa: BLE001 -- harness-side fault; end only this run print(f"[SIM-USER] Simulated reply failed for conversation {conv_id}: {exc}") @@ -779,7 +885,8 @@ 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, sdk=sdk), + recipients_correct=tool_called + and _check_recipients(expected, actual_args, sdk=sdk, current_user_id=current_user_id), attributes_correct=tool_called and _check_attributes(expected, actual_args), granularity_correct=tool_called and _check_granularity(expected, actual_args), ) diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index faad70f84..ef1fa33c4 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -15,6 +15,7 @@ _check_trigger, _deep_subset, _normalize_expected_output, + _resolve_current_user_id, _to_number, evaluate_agentic_alert_skill, generate_simulated_alert_response, @@ -289,6 +290,202 @@ def test_check_recipients_resolution_failure_fails_gracefully(): assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False +def test_check_recipients_logs_when_no_user_has_the_expected_email(capsys: pytest.CaptureFixture[str]) -> None: + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [] + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False + out = capsys.readouterr().out + assert "No user has any of the 1 recipient email(s)" in out + assert "user@example.com" not in out + + +def test_check_recipients_logs_a_failed_email_lookup(capsys: pytest.CaptureFixture[str]) -> None: + 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 + assert "failed: boom" in capsys.readouterr().out + + +# --- Current user as the recipient ------------------------------------------------------ + + +@pytest.mark.parametrize("spelling", ["me", "Me", " current user ", "current_user", "myself"]) +def test_current_user_recipient_spellings_normalize_to_me(spelling: str) -> None: + assert _normalize_expected_output({"Recipient(s)": spelling}).recipients == ["me"] + + +def test_check_recipients_accepts_the_current_user_by_id() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me"}) + mock_sdk = MagicMock() + assert _check_recipients(expected, {"internal_recipients": ["admin"]}, sdk=mock_sdk, current_user_id="admin") + assert _check_recipients(expected, {"internal_recipients": "admin"}, sdk=mock_sdk, current_user_id="admin") + mock_sdk._client.entities_api.get_all_entities_users.assert_not_called() + + +@pytest.mark.parametrize( + "actual_args", + [ + {"internal_recipients": ["someone.else"]}, + {"external_recipients": ["admin@gooddata.com"]}, + {"internal_recipients": ["admin", "someone.else"]}, + {"internal_recipients": ["admin"], "external_recipients": ["admin@gooddata.com"]}, + {}, + ], +) +def test_check_recipients_rejects_anything_but_exactly_the_current_user(actual_args: dict) -> None: + expected = _normalize_expected_output({"Recipient(s)": "me"}) + assert _check_recipients(expected, actual_args, sdk=MagicMock(), current_user_id="admin") is False + + +def test_check_recipients_fails_when_the_current_user_is_unknown() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me"}) + assert _check_recipients(expected, {"internal_recipients": ["admin"]}, current_user_id=None) is False + + +def test_check_recipients_accepts_the_current_user_alongside_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 + assert _check_recipients(expected, {"internal_recipients": ["admin"]}, current_user_id="admin") is False + extra = {"internal_recipients": ["admin", "carol.id"], "external_recipients": ["bob@example.com"]} + assert _check_recipients(expected, extra, 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() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [MagicMock(id="bob.id")] + args = {"internal_recipients": ["admin", "bob.id"]} + assert _check_recipients(expected, args, sdk=mock_sdk, current_user_id="admin") is True + mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(filter="email=in=('bob@example.com')") + + +def test_check_recipients_rejects_a_user_id_that_matches_no_expected_email() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me, bob@example.com"}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [MagicMock(id="bob.id")] + args = {"internal_recipients": ["admin", "bob.id", "carol.id"]} + assert _check_recipients(expected, args, sdk=mock_sdk, current_user_id="admin") is False + + +def test_check_recipients_rejects_an_expected_email_that_resolves_to_no_user() -> None: + expected = _normalize_expected_output({"Recipient(s)": "me, bob@example.com, carol@example.com"}) + mock_sdk = MagicMock() + bob, nobody = MagicMock(data=[MagicMock(id="bob.id")]), MagicMock(data=[]) + mock_sdk._client.entities_api.get_all_entities_users.side_effect = [bob, nobody] + args = {"internal_recipients": ["admin", "bob.id"]} + assert _check_recipients(expected, args, sdk=mock_sdk, current_user_id="admin") is False + + +def test_resolve_current_user_id_reads_the_profile() -> None: + with patch("gooddata_eval.core.agentic.alert_skill.httpx.get") as mock_get: + mock_get.return_value.json.return_value = {"userId": "admin", "organizationId": "org"} + assert _resolve_current_user_id("http://host/", "tok") == "admin" + mock_get.assert_called_once_with( + "http://host/api/v1/profile", headers={"Authorization": "Bearer tok"}, timeout=30.0 + ) + + +def test_resolve_current_user_id_logs_and_returns_none_on_failure(capsys: pytest.CaptureFixture[str]) -> None: + with patch("gooddata_eval.core.agentic.alert_skill.httpx.get", side_effect=RuntimeError("down")): + assert _resolve_current_user_id("http://host", "tok") is None + assert "Resolving the current user failed: down" in capsys.readouterr().out + + +def test_sim_user_for_a_current_user_alert_gives_no_email() -> None: + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Recipient(s)": "me"}, current_user_id="admin") + assert "recipients=me (yourself, the current user, user id admin)" in prompt + assert "Never give an email address" in prompt + assert "'You'" in prompt + assert "Proactively include your email recipient" not in prompt + + +def test_sim_user_still_volunteers_an_email_recipient() -> None: + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Recipient(s)": "bob@example.com"}) + assert "recipients=bob@example.com" in prompt + assert "Proactively include your email recipient" in prompt + + +def test_run_agentic_alert_skill_scores_a_current_user_alert() -> None: + asked_turn = ChatResult.model_validate({"text_response": "Which dashboard?", "tool_call_events": []}) + created_turn = ChatResult.model_validate( + { + "text_response": "Alert created.", + "tool_call_events": [ + { + "functionName": "create_metric_alert", + "functionArguments": '{"operator": "LESS_THAN", "threshold": 30000, "internal_recipients": ["admin"]}', + "result": '{"id": "alert-1"}', + } + ], + } + ) + mock_client = MagicMock() + mock_client.send_message.side_effect = [asked_turn, created_turn] + + with ( + _patched(mock_client, simulated_reply="Any dashboard is fine.", delete_alert=True) as mock_sim, + patch("gooddata_eval.core.agentic.alert_skill._resolve_current_user_id", return_value="admin") as mock_me, + ): + summary = run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Alert me when profit drops below 30000.", + expected_output={"Operator": "LESS_THAN", "Threshold": "30000", "Recipient(s)": "me"}, + k=1, + max_iterations=6, + initial_conversation_id="conv-1", + ) + + mock_me.assert_called_once_with("http://host", "tok") + assert mock_sim.call_args.kwargs["current_user_id"] == "admin" + assert summary.best.eval.recipients_correct is True + + +def test_run_agentic_alert_skill_skips_the_profile_lookup_for_email_recipients() -> None: + mock_client = MagicMock() + mock_client.send_message.return_value = _no_alert_chat_result() + with ( + _patched(mock_client, simulated_reply="ok"), + patch("gooddata_eval.core.agentic.alert_skill._resolve_current_user_id") as mock_me, + ): + run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Alert me", + expected_output={"Operator": "LESS_THAN", "Recipient(s)": "admin@gooddata.com"}, + k=1, + max_iterations=1, + initial_conversation_id="conv-1", + ) + mock_me.assert_not_called() + + +def test_run_agentic_alert_skill_errors_when_the_current_user_cannot_be_resolved() -> None: + mock_client = MagicMock() + with ( + _patched(mock_client, simulated_reply="ok") as mock_sim, + patch("gooddata_eval.core.agentic.alert_skill._resolve_current_user_id", return_value=None), + pytest.raises(RuntimeError, match="user id could not be resolved"), + ): + run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Alert me", + expected_output={"Operator": "LESS_THAN", "Recipient(s)": "me"}, + k=1, + max_iterations=1, + initial_conversation_id="conv-1", + ) + mock_client.send_message.assert_not_called() + mock_sim.assert_not_called() + + def test_alert_evaluation_strict_pass(): ev = AlertEvaluation( alert_created=True, @@ -382,7 +579,7 @@ def test_run_agentic_alert_skill_creates_fresh_conversations_for_remaining_runs( # summary that plainly read "Trigger: once per month". -def _sim_user_prompt(expected_output: dict, question: str = "") -> str: +def _sim_user_prompt(expected_output: dict, question: str = "", current_user_id: str | None = None) -> str: """Run the sim-user against a stub OpenAI client and return the system prompt it built.""" fake_openai = MagicMock() fake_openai.return_value.chat.completions.create.return_value = MagicMock( @@ -397,6 +594,7 @@ def _sim_user_prompt(expected_output: dict, question: str = "") -> str: _normalize_expected_output(expected_output), [], question=question, + current_user_id=current_user_id, ) call = fake_openai.return_value.chat.completions.create.call_args return call.kwargs["messages"][0]["content"] @@ -942,6 +1140,16 @@ def test_sim_user_still_refuses_an_unrequested_granularity_for_a_normal_alert(): assert "no date filter at all" in prompt +@pytest.mark.parametrize("operator", ["INCREASES_BY", "DECREASES_BY", "CHANGES_BY"]) +def test_sim_user_delegates_the_comparison_period_of_a_relative_alert(operator: str) -> None: + prompt = _sim_user_prompt({"Operator": operator, "Time window/Filters": "None (All time)"}) + rule_7 = prompt.split("7.", 1)[1] + assert "NOT a date filter" in rule_7 + assert "never applies to the comparison" in rule_7 + assert "Pick whichever date and period fit best" in rule_7 + assert "Do not invent an evaluation period, a granularity" not in prompt + + def test_sim_user_falls_back_to_day_when_an_anomaly_item_states_no_granularity(): prompt = _sim_user_prompt({"Operator": "ANOMALY"}) assert "use DAY granularity" in prompt