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 e75a2bed2..42ce9f3ec 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 @@ -27,6 +27,13 @@ _TRIGGER_DISPLAY_TO_API = {"Every time": "ALWAYS", "One time": "ONCE"} _ALWAYS_TRIGGER_VALUES = {"Every time", "ALWAYS", "not specified"} +_TRIGGER_INSTRUCTIONS = { + "ALWAYS": ( + "alert me EVERY TIME the condition is met — not once per day, week or month, and not only the first time" + ), + "ONCE": "alert me ONLY THE FIRST TIME the condition is met, then stop", +} + def _to_number(value: object) -> float | int | None: """Convert string/number to int or float, None on failure.""" @@ -94,9 +101,11 @@ def _check_trigger(expected: CatalogMetricAlert, actual_args: dict) -> bool: def _check_filters(expected: CatalogMetricAlert, actual_args: dict) -> bool: exp_filters = expected.filters - act_filters = actual_args.get("filters", actual_args.get("attribute_filters")) - if not exp_filters: + act_filters = actual_args.get("filters", actual_args.get("attribute_filters")) or [] + if exp_filters is None: return True + if not exp_filters: + return not act_filters if not act_filters: return False return _deep_subset(exp_filters, act_filters) @@ -132,8 +141,15 @@ def generate_simulated_alert_response( agent_message: str, expected: CatalogMetricAlert, conversation_history: list, + question: str = "", ) -> str: - """Stateful sim-user reply for alert-skill conversation (gpt-4o).""" + """Stateful sim-user reply for alert-skill conversation (gpt-4o). + + ``question`` is the fixture's original request. The sim-user is first called with an empty + 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. + """ if _OpenAI is None: raise RuntimeError( "openai package is required for generate_simulated_alert_response. " @@ -147,30 +163,83 @@ def generate_simulated_alert_response( metric = expected.metric_id or "not specified" operator = expected.operator - threshold = expected.threshold if expected.threshold is not None else "not specified" + # BETWEEN / NOT_BETWEEN carry their value in threshold_from/threshold_to, so `threshold` is + # None for them. Rule 3 asks the sim-user to verify the threshold, and reporting "not + # specified" made it demand the agent delete both bounds of a BETWEEN condition — an + # impossible request that burned every iteration without the alert ever being created. + threshold: str | float | int + if expected.operator in ("BETWEEN", "NOT_BETWEEN") and ( + expected.threshold_from is not None or expected.threshold_to is not None + ): + threshold = f"between {expected.threshold_from} and {expected.threshold_to}" + elif expected.threshold is not None: + threshold = expected.threshold + else: + threshold = "not specified" recipients = ", ".join(expected.recipients) if expected.recipients else "not specified" trigger = expected.trigger filters = expected.filters - trigger_line = ( - f"5. Proactively tell the agent the trigger is '{trigger}' in your first reply.\n" - if trigger not in _ALWAYS_TRIGGER_VALUES - else "" - ) + # "not specified" is the normalizer's stand-in for an absent trigger, which the product + # persists as its ALWAYS default and `_check_trigger` asserts as ALWAYS. Both the cadence to + # ask for (rule 6) and the goal text (rule 1) use the resolved value: reporting the raw + # placeholder made rule 3 treat the trigger as unconstrained, so the sim-user would confirm a + # ONCE/ONCE_PER_INTERVAL proposal that the assertion then failed. + trigger_key = "ALWAYS" if trigger in _ALWAYS_TRIGGER_VALUES else trigger + trigger_request = _TRIGGER_INSTRUCTIONS.get(trigger_key, f"set the trigger to {trigger}") + + # Three branches, matching the three states of `expected.filters`. `[]` and `None` must not + # share one: telling the sim-user "you want NO filters" on an unstated expectation makes it + # refuse filters the request genuinely implies (e.g. "orders from the United States"), which + # quietly turns that fixture into a weaker test rather than a failing one. + if filters: + filters_rule = ( + f"5. Your alert needs exactly these filters and NOTHING else: {filters}. " + "If the agent offers, proposes or asks about any further date/time window, " + "evaluation period or granularity, refuse it and repeat that these are the only " + "filters you want.\n" + ) + elif filters == []: + filters_rule = ( + "5. Your alert must have NO filters and NO date/time window — it evaluates over all time. " + "If the agent asks which time period each check should cover, or offers a choice such as " + "'last Day / Week / Month', do NOT pick one: reply that you want no date filter at all, " + "all time. Never invent a period, a granularity or an 'evaluate each run on a X basis' " + "instruction the goal did not ask for.\n" + ) + else: + filters_rule = ( + "5. Ask only for the filters your original request implies — do not invent an evaluation " + "period, granularity or date window that was not requested. If the agent offers a choice " + "such as 'last Day / Week / Month' that your request never mentioned, say you do not want " + "a date window.\n" + ) + + original_request = f'Your original request to the agent was: "{question}"\n' if question else "" + system_prompt = ( "You are a user requesting creation of an alert for a metric from an AI agent. " "Respond naturally but always steer toward the exact values you were given.\n" - "Rules you MUST follow:\n" + + original_request + + "Rules you MUST follow:\n" f"1. Your goal: metric={metric}, operator={operator}, threshold={threshold}, " - f"recipients={recipients}, trigger={trigger}" + (f", filters={filters}" if filters else "") + ".\n" + f"recipients={recipients}, trigger={trigger_key}" + (f", filters={filters}" if filters else "") + ".\n" "2. Never revert or change a decision that was already confirmed in a previous turn.\n" - "3. If the agent shows a final summary and asks for confirmation, verify that the " - " recipients match your goal. If they differ, correct them. " - " Once recipients are correct, say 'Yes, please proceed to create the alert.'\n" + "3. If the agent shows a final summary, an alert proposal or asks for confirmation, check " + " ALL of these against your goal: recipients, trigger (how often you are alerted), " + " filters / time window, threshold and operator. If ANY of them differs — for example the " + " summary says 'once per day/week/month' but your goal is every time, or it lists a date " + " filter you never asked for — do NOT confirm: name the wrong field, state the correct " + " value and ask the agent to fix it. Say 'Yes, please proceed to create the alert.' ONLY " + " 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" - + trigger_line - + "Reply concisely and directly." + + 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" + "Reply concisely and directly." ) messages: list = [{"role": "system", "content": system_prompt}] @@ -257,6 +326,29 @@ def _case_insensitive_get(d: dict, *keys: str) -> Any: return None +_NO_FILTER_MARKERS = ("none", "all time") + + +def _normalize_expected_filters(expected: dict) -> list | str | None: + """ + * ``Filters`` list -> that list (exact expectation) + * "None (All time)" in either -> ``[]`` (stated: no filters; extras fail) + * anything else / absent -> ``None`` (unstated; filters not asserted) + """ + filters = _case_insensitive_get(expected, "filters") + if isinstance(filters, list): + return filters + time_window = _case_insensitive_get(expected, "time window/filters", "time_window") + for candidate in (filters, time_window): + if isinstance(candidate, str) and any(kw in candidate.lower() for kw in _NO_FILTER_MARKERS): + return [] + # Prose that is not a no-filter marker ("Product Category = X") describes a filter without + # encoding it, so it cannot be compared: returning it made `_check_filters` fall through to + # `_deep_subset(str, list)`, which can never match. `None` is what the contract above + # promises — the sim-user derives such filters from the original request instead. + return None + + def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: """Parse expected_output dict into CatalogMetricAlert, accepting display-format or internal-format keys.""" operator = _case_insensitive_get(expected, "operator") or "GREATER_THAN" @@ -280,9 +372,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: else: recipients = list(raw_recip) - filters = _case_insensitive_get(expected, "filters") - if isinstance(filters, str) and any(kw in filters for kw in ("None", "All time")): - filters = None + filters = _normalize_expected_filters(expected) return CatalogMetricAlert( operator=operator, @@ -377,7 +467,9 @@ def _run_once(conv_id: str) -> AlertRunResult: # Stop before generating a follow-up for the last iteration if _iteration >= max_iterations - 1: break - follow_up = generate_simulated_alert_response(response_text, expected, conversation_history) + follow_up = generate_simulated_alert_response( + response_text, expected, conversation_history, question=question + ) # Record this exchange so the next call has full history conversation_history.append({"role": "assistant", "content": response_text}) conversation_history.append({"role": "user", "content": follow_up}) diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index fa5dcbedd..13c94c2bb 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -4,15 +4,32 @@ from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, + _check_filters, _check_trigger, _deep_subset, _normalize_expected_output, _to_number, + generate_simulated_alert_response, render_alert_proposal, run_agentic_alert_skill, ) from gooddata_eval.core.models import ChatResult +_DATE_FILTER = { + "relativeDateFilter": { + "dataset": {"identifier": {"id": "order_date", "type": "dataset"}}, + "granularity": "MONTH", + "from": -1, + "to": -1, + } +} +_ATTR_FILTER = { + "positiveAttributeFilter": { + "label": {"identifier": {"id": "customer_country", "type": "label"}}, + "in": {"values": ["United States"]}, + } +} + _PROPOSAL = { "title": "# of Orders Alert - Greater Than 500", "cta": "Should I create this alert?", @@ -64,6 +81,66 @@ def test_check_trigger_once_needs_explicit_once(): assert _check_trigger(expected, {"trigger": "ONCE_PER_INTERVAL"}) is False # real model error stays a fail +# --- filters: "no filters" is an expectation, "unspecified" is not (QA-28623) --------------- +# +# `_check_filters` used to return True whenever the expectation was empty, so an alert that +# bolted on an unrequested relativeDateFilter scored filters_correct=1 and the drift the +# ticket is about was invisible in the eval and on the trace. + + +def test_check_filters_stated_none_rejects_extra_date_filter(): + expected = _normalize_expected_output({"Operator": "GREATER_THAN", "Time window/Filters": "None (All time)"}) + assert expected.filters == [] # stated, not merely absent + assert _check_filters(expected, {"filters": []}) is True + assert _check_filters(expected, {}) is True + assert _check_filters(expected, {"filters": [_DATE_FILTER]}) is False + + +def test_check_filters_unspecified_is_not_asserted(): + # Prose-only expectation: describes a filter the alert must have, but not comparably. + # Demanding emptiness here would fail an alert whose filters are in fact correct. + expected = _normalize_expected_output( + {"Operator": "LESS_THAN", "Time window/Filters": "Customer Country = United States"} + ) + assert expected.filters is None + assert _check_filters(expected, {"filters": [_ATTR_FILTER, _DATE_FILTER]}) is True + + +def test_check_filters_absent_time_window_is_not_asserted(): + expected = _normalize_expected_output({"Operator": "ANOMALY"}) + assert expected.filters is None + assert _check_filters(expected, {"filters": [_DATE_FILTER]}) is True + + +def test_check_filters_explicit_list_still_requires_subset(): + expected = _normalize_expected_output({"Operator": "LESS_THAN", "Filters": [_ATTR_FILTER]}) + assert _check_filters(expected, {"filters": [_ATTR_FILTER]}) is True + assert _check_filters(expected, {"filters": []}) is False + # An extra filter beyond the expected list is still a length mismatch -> fail. + assert _check_filters(expected, {"filters": [_ATTR_FILTER, _DATE_FILTER]}) is False + + +def test_normalize_expected_filters_prefers_machine_readable_list(): + # Both columns present: the list wins over the prose, which merely paraphrases it. + expected = _normalize_expected_output( + {"Operator": "LESS_THAN", "Filters": [_ATTR_FILTER], "Time window/Filters": "Customer Country = United States"} + ) + assert expected.filters == [_ATTR_FILTER] + + +def test_normalize_expected_filters_reads_none_marker_from_filters_column(): + expected = _normalize_expected_output({"Operator": "LESS_THAN", "Filters": "None (All time)"}) + assert expected.filters == [] + + +def test_normalize_expected_filters_treats_prose_filters_column_as_unspecified(): + # Prose in `Filters` used to be returned verbatim, so `_check_filters` compared a string to a + # list of filter dicts and could never pass — a guaranteed failure for a correct alert. + expected = _normalize_expected_output({"Operator": "LESS_THAN", "Filters": "Product Category = X"}) + assert expected.filters is None + assert _check_filters(expected, {"filters": [_ATTR_FILTER]}) is True + + def test_alert_evaluation_strict_pass(): ev = AlertEvaluation( alert_created=True, @@ -171,6 +248,170 @@ def test_run_agentic_alert_skill_creates_fresh_conversations_for_remaining_runs( assert mock_client.delete_conversation.call_count == 2 +# --- simulated user prompt (QA-28623) -------------------------------------------------------- +# +# The drift these rules guard against was the sim-user's, not the agent's: asked "what time +# window should each check use? Day / Week / Month" it volunteered "monthly", then confirmed a +# summary that plainly read "Trigger: once per month". + + +def _sim_user_prompt(expected_output: dict, question: str = "") -> 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( + choices=[MagicMock(message=MagicMock(content="ok"))] + ) + with ( + patch("gooddata_eval.core.agentic.alert_skill._OpenAI", fake_openai), + patch.dict("os.environ", {"OPENAI_API_KEY": "test-key"}), + ): + generate_simulated_alert_response( + "What time period should each check cover?", + _normalize_expected_output(expected_output), + [], + question=question, + ) + call = fake_openai.return_value.chat.completions.create.call_args + return call.kwargs["messages"][0]["content"] + + +def test_sim_user_states_always_trigger_in_natural_language(): + # `trigger=ALWAYS` alone left the sim-user silent about cadence; it must now ask for it + # in words a role-playing user would actually use. + prompt = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + assert "EVERY TIME" in prompt + assert "not once per day, week or month" in prompt + + +def test_sim_user_asks_for_always_cadence_when_fixture_omits_trigger(): + # A fixture with no Trigger still demands ALWAYS (the product default `_check_trigger` + # asserts), so rule 6 must ask for it rather than echo the "not specified" placeholder. + prompt = _sim_user_prompt({"Operator": "GREATER_THAN"}) + assert "EVERY TIME" in prompt + + +def test_sim_user_states_once_trigger_in_natural_language(): + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Trigger": "One time"}) + assert "ONLY THE FIRST TIME" in prompt + + +def test_sim_user_goal_renders_omitted_trigger_as_always(): + # Rule 3 tells the sim-user to accept fields the goal reports as "not specified". Leaving the + # trigger placeholder in the goal therefore licensed it to confirm a ONCE / ONCE_PER_INTERVAL + # proposal — which `_check_trigger` then fails, because an omitted trigger means ALWAYS. + expected = {"Operator": "GREATER_THAN"} + prompt = _sim_user_prompt(expected) + assert "trigger=ALWAYS" in prompt + assert "trigger=not specified" not in prompt + assert _check_trigger(_normalize_expected_output(expected), {"trigger": "ONCE"}) is False + + +def test_sim_user_prompt_carries_the_original_request(): + # The opening question goes straight to the agent, so the sim-user's first call has an empty + # history. Rule 5's "the filters your original request implies" needs the request in view. + question = "Notify me when the number of orders from the United States falls below 100" + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Threshold": "100"}, question=question) + assert question in prompt + + +def test_run_agentic_alert_skill_passes_question_to_sim_user(): + # Interaction: the agent asks for a filter the fixture never restates, so the sim-user can + # only supply it by reading the original request out of its own prompt. + question = "Notify me when the number of orders from the United States falls below 100" + asked_turn = ChatResult.model_validate( + {"text_response": "Which country should the alert filter on?", "tool_call_events": []} + ) + created_turn = ChatResult.model_validate( + { + "text_response": "Alert created.", + "tool_call_events": [ + { + "functionName": "create_metric_alert", + "functionArguments": '{"operator": "LESS_THAN", "threshold": 100}', + "result": '{"id": "alert-1"}', + } + ], + } + ) + mock_client = MagicMock() + mock_client.send_message.side_effect = [asked_turn, created_turn] + + with ( + patch("gooddata_eval.core.agentic.alert_skill.ChatClient", return_value=mock_client), + patch( + "gooddata_eval.core.agentic.alert_skill.generate_simulated_alert_response", + return_value="United States.", + ) as mock_sim, + patch("gooddata_eval.core.agentic.alert_skill._delete_alert"), + ): + run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question=question, + expected_output={"operator": "LESS_THAN", "threshold": 100}, + k=1, + max_iterations=6, + initial_conversation_id="conv-1", + ) + + assert mock_sim.call_args.kwargs["question"] == question + # The agent message stays positional so existing callers/patches keep working. + assert mock_sim.call_args.args[0] == "Which country should the alert filter on?" + + +def test_sim_user_goal_states_between_bounds(): + # BETWEEN keeps its value in threshold_from/to, so `threshold` is None. Reporting the goal + # as "not specified" made the sim-user demand the agent delete both bounds — impossible, so + # it looped until max_iterations and the alert was never created (gpt56luna run, item _5). + prompt = _sim_user_prompt( + {"Operator": "BETWEEN", "Threshold_from": 50000, "Threshold_to": 200000, "Trigger": "Every time"} + ) + assert "threshold=between 50000 and 200000" in prompt + assert "threshold=not specified" not in prompt + + +def test_sim_user_accepts_fields_the_goal_leaves_unspecified(): + # Rule 3 must not turn an absent expectation into a correction demand. + prompt = _sim_user_prompt({"Operator": "ANOMALY"}) + assert "'not specified' is one you have NO expectation about" in prompt + assert "never ask for it to be removed" in prompt + # The phrase must survive literal concatenation intact — a line break mid-sentence used to + # render it as "'not specified'", which the sim-user reads as a different instruction. + assert "'not specified'" not in prompt + + +def test_sim_user_refuses_invented_time_window_when_no_filters_expected(): + prompt = _sim_user_prompt({"Operator": "GREATER_THAN", "Time window/Filters": "None (All time)"}) + assert "NO filters" in prompt + assert "last Day / Week / Month" in prompt + assert "all time" in prompt + + +def test_sim_user_is_not_told_no_filters_when_expectation_is_unstated(): + # Prose-only expectation normalizes to None, not []. Claiming "NO filters" there would make + # the sim-user refuse the country filter this request genuinely implies — the fixture would + # still pass (filters are not asserted) while testing much less than it looks like. + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Time window/Filters": "Customer Country = United States"}) + assert "NO filters" not in prompt + assert "do not invent an evaluation period" in prompt + + +def test_sim_user_refuses_extra_filters_when_filters_expected(): + prompt = _sim_user_prompt({"Operator": "LESS_THAN", "Filters": [_ATTR_FILTER]}) + assert "NOTHING else" in prompt + assert "refuse it" in prompt + + +def test_sim_user_verifies_trigger_and_filters_before_confirming(): + # Rule 3 checked recipients only, so a summary showing the wrong trigger was rubber-stamped. + prompt = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + rule_3 = prompt.split("3.", 1)[1].split("4.", 1)[0] + for field in ("recipients", "trigger", "filters", "threshold", "operator"): + assert field in rule_3, f"final-summary check must cover {field}" + assert "do NOT confirm" in rule_3 + + def test_render_alert_proposal_keeps_verifiable_fields_and_drops_afm(): rendered = render_alert_proposal(_PROPOSAL) # The CTA leads so the simulated user reads it as a question.