From dbbe6c7efaa3eed137a2d24207b9dda91d162e42 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 30 Jul 2026 17:47:15 +0700 Subject: [PATCH] fix(gooddata-eval): keep alert sim-user from accepting trigger drift The staging nightly repeatedly failed agent_alert_skill_2 and _15: the agent created ONCE_PER_INTERVAL/DAY alerts where the fixture expects ALWAYS, plus date filters nobody asked for. The chatbot was following the simulated user, whose prompt had three gaps: - The expected trigger was stated only when it differed from ALWAYS, so exactly the ALWAYS cases went unsaid. The alert prompt tells the agent to align trigger_interval with any date granularity in play "unless the user explicitly chooses a different interval", so an ALWAYS expectation only survives when the sim-user asks for it out loud. Now stated for every explicit trigger, and still silent both when the fixture omits Trigger (those cases exercise the product default) and for ANOMALY, whose product default really is ONCE_PER_INTERVAL. - The final-summary verification rule checked recipients only, so a summary showing a wrong trigger got confirmed anyway. It checked the trigger too until the logic moved into this SDK; restored, and extended to filters. - Nothing forbade extra filters. The agent volunteers a date filter for a vague question and then aligns the trigger interval to it, so refusing the filter removes the reason it had to change the trigger in the first place. Fixing the trigger rule alone would be fragile. Sampling temperature drops to 0 as well, since the drift was sampling-dependent. Each Langfuse score now carries expected-vs-actual detail. These comments also existed before the same migration and were lost with it: without them a failed trigger/filters check shows only 0.0 on the trace, and the create_metric_alert arguments have to be dug out of CI logs or of nested observations on a different trace of the session than the one the scores land on. JIRA: QA-28623 risk: nonprod --- .../gooddata_eval/core/agentic/alert_skill.py | 149 ++++++++++++--- .../tests/test_agentic_alert_skill.py | 169 ++++++++++++++++++ 2 files changed, 297 insertions(+), 21 deletions(-) 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 3d3adb15a..b8415169d 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 @@ -25,6 +25,17 @@ _TRIGGER_DISPLAY_TO_API = {"Every time": "ALWAYS", "One time": "ONCE"} _ALWAYS_TRIGGER_VALUES = {"Every time", "ALWAYS", "not specified"} +_TRIGGER_UNSPECIFIED = "not specified" +_RANGE_OPERATORS = ("BETWEEN", "NOT_BETWEEN") + +# Plain-language phrasing of each expected trigger, for the sim-user to demand out loud. +_TRIGGER_INSTRUCTIONS = { + "ALWAYS": ( + "alert me EVERY TIME the condition is met — not once per day, week, month, quarter or year, " + "and do not set any trigger interval" + ), + "ONCE": "alert me ONLY THE FIRST TIME the condition is met, then stop", +} def _to_number(value: object) -> float | int | None: @@ -65,7 +76,7 @@ def _deep_subset(expected: object, actual: object) -> bool: def _check_threshold(expected: CatalogMetricAlert, actual_args: dict) -> bool: - if expected.operator in ("BETWEEN", "NOT_BETWEEN"): + if expected.operator in _RANGE_OPERATORS: exp_from = _to_number(expected.threshold_from) exp_to = _to_number(expected.threshold_to) act_from = _to_number(actual_args.get("from_value", actual_args.get("fromValue"))) @@ -109,9 +120,8 @@ def _check_metric(expected: CatalogMetricAlert, actual_args: dict) -> bool: return expected.metric_id == act_metric -def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: - if not expected.recipients: - return True +def _actual_recipients(actual_args: dict) -> list: + """Recipient list from the tool arguments, whatever shape the agent used.""" act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) if isinstance(act_recip_raw, str): # external_recipients is JSON-encoded (e.g. '["email@example.com"]') @@ -124,7 +134,49 @@ 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 []) + return act_recip or [] + + +def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: + if not expected.recipients: + return True + return set(expected.recipients) == set(_actual_recipients(actual_args)) + + +def _trigger_instruction(expected: CatalogMetricAlert) -> str: + """What the sim-user must demand about the trigger, or '' when it should stay silent. + + Stated for every explicit trigger, ALWAYS included: the product prompt tells the agent to + align `trigger_interval` with any date granularity in play, so an ALWAYS expectation only + survives when the user asks for it out loud. Silent when the fixture omits Trigger (so those + cases still exercise the product default) and for ANOMALY, whose product default really is + ONCE_PER_INTERVAL and whose trigger the assertion does not check. + """ + if expected.operator == "ANOMALY" or expected.trigger == _TRIGGER_UNSPECIFIED: + return "" + return _TRIGGER_INSTRUCTIONS.get(expected.trigger, f"set the trigger to '{expected.trigger}'") + + +def _filter_rule(filters: list | str | None) -> str: + """Rule keeping the sim-user from accepting filters the fixture did not ask for. + + The agent volunteers a date filter for vague questions and then aligns the trigger interval + to it, which is how an expected-ALWAYS case drifts to ONCE_PER_INTERVAL. Refusing the extra + filter removes the reason the agent had to change the trigger. + """ + if filters: + return ( + f"The ONLY filters this alert may have are: {filters}. Do NOT accept any additional " + "filter the agent proposes — in particular no extra date, time-window or period " + "filter. If its proposal or summary adds one, tell it to drop the extra filter and " + "keep only the filters listed here." + ) + return ( + "This alert must have NO filters at all. If the agent asks about a time window, date " + "range or period, answer 'All time — no date range filter'. Do NOT accept any date, " + "time-window or period filter the agent proposes; if its proposal or summary adds one, " + "tell it to drop the filter." + ) def generate_simulated_alert_response( @@ -151,24 +203,26 @@ def generate_simulated_alert_response( 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 "" - ) + rules = [ + f"Your goal: metric={metric}, operator={operator}, threshold={threshold}, " + f"recipients={recipients}, trigger={trigger}" + (f", filters={filters}" if filters else "") + ".", + "Never revert or change a decision that was already confirmed in a previous turn.", + "If the agent shows a proposal or final summary and asks for confirmation, verify that the " + "recipients, the trigger AND the filters all match your goal. Correct every mismatch in " + "your reply. Only once all three are correct, say 'Yes, please proceed to create the alert.'", + "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.", + ] + trigger_instruction = _trigger_instruction(expected) + if trigger_instruction: + rules.append(f"Proactively tell the agent in your first reply: {trigger_instruction}.") + rules.append(_filter_rule(filters)) + 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" - f"1. Your goal: metric={metric}, operator={operator}, threshold={threshold}, " - f"recipients={recipients}, trigger={trigger}" + (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" - "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 + + "".join(f"{i}. {rule}\n" for i, rule in enumerate(rules, start=1)) + "Reply concisely and directly." ) @@ -180,7 +234,9 @@ def generate_simulated_alert_response( response = openai_client.chat.completions.create( model="gpt-4o", messages=messages, - temperature=0.5, + # Deterministic as the model allows: the trigger/filter drift this prompt guards against + # was sampling-dependent, so temperature must not reintroduce it. + temperature=0, ) return response.choices[0].message.content or "" @@ -424,6 +480,48 @@ def _run_once(conv_id: str) -> AlertRunResult: ) +def _actual_trigger_display(actual_args: dict) -> str: + """Trigger as the agent set it, with the interval appended when one applies.""" + trigger = actual_args.get("trigger") or actual_args.get("triggerMode") or "ALWAYS" + interval = actual_args.get("trigger_interval") or actual_args.get("triggerInterval") + return f"{trigger}/{interval}" if interval else str(trigger) + + +def _score_comments(expected: CatalogMetricAlert, actual_args: dict, alert_created: bool) -> dict[str, str]: + """Expected-vs-actual detail per strict check, for the Langfuse score comments. + + Without these a failed check shows only 0.0 on the trace, and triaging means reading CI logs + or hunting the create_metric_alert arguments through nested trace observations. + """ + if not alert_created: + return {"alert_created": "create_metric_alert was never called"} + + if expected.operator in _RANGE_OPERATORS: + exp_threshold: object = f"{expected.threshold_from}..{expected.threshold_to}" + act_threshold: object = ( + f"{actual_args.get('from_value', actual_args.get('fromValue'))}.." + f"{actual_args.get('to_value', actual_args.get('toValue'))}" + ) + else: + exp_threshold = expected.threshold + act_threshold = actual_args.get("threshold") + + act_metric_raw = str(actual_args.get("metric_id", actual_args.get("metricId", ""))) + act_metric = _parse_metric_id(act_metric_raw) or act_metric_raw + return { + "alert_created": "create_metric_alert was called", + "operator_correct": f"expected {expected.operator!r}; actual {actual_args.get('operator')!r}", + "threshold_correct": f"expected {exp_threshold!r}; actual {act_threshold!r}", + "trigger_correct": f"expected {expected.trigger!r}; actual {_actual_trigger_display(actual_args)!r}", + "filters_correct": ( + f"expected {expected.filters!r}; " + f"actual {actual_args.get('filters', actual_args.get('attribute_filters'))!r}" + ), + "metric_correct": f"expected {expected.metric_id!r}; actual {act_metric!r}", + "recipients_correct": f"expected {expected.recipients!r}; actual {_actual_recipients(actual_args)!r}", + } + + class AlertSkillAssertionError(AssertionError): """Raised when an alert-skill evaluation fails.""" @@ -489,6 +587,7 @@ def evaluate_agentic_alert_skill( [r.conversation_id for r in summary.run_results], window_start, ) + expected = _normalize_expected_output(expected_output) suffix_needed = len(summary.run_results) > 1 for run_idx, run in enumerate(summary.run_results): pt = traces_by_conv.get(run.conversation_id) @@ -503,9 +602,17 @@ def evaluate_agentic_alert_skill( "metric_correct": ev.metric_correct, "recipients_correct": ev.recipients_correct, } + comments = _score_comments(expected, run.actual_alert_arguments, ev.alert_created) with observe(langfuse, pt.id if pt else None, dataset_item_id, run_name, run_metadata) as tid: for score_name, value in strict_checks.items(): - score_safe(langfuse, tid, name=score_name, value=float(value), data_type="BOOLEAN") + score_safe( + langfuse, + tid, + name=score_name, + value=float(value), + data_type="BOOLEAN", + comment=comments.get(score_name), + ) log_quality_and_value_scores( langfuse, tid, diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index d6026da04..7691cae41 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -1,18 +1,37 @@ # (C) 2026 GoodData Corporation. All rights reserved. # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise +import json from unittest.mock import MagicMock, patch +import pytest from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, + AlertSkillAssertionError, _check_trigger, _deep_subset, _normalize_expected_output, + _score_comments, _to_number, + evaluate_agentic_alert_skill, + generate_simulated_alert_response, run_agentic_alert_skill, ) from gooddata_eval.core.models import ChatResult +def _sim_user_prompt(expected_output: dict) -> tuple[str, dict]: + """Render the sim-user system prompt for a fixture, plus the kwargs sent to gpt-4o.""" + mock_openai = MagicMock() + mock_openai.return_value.chat.completions.create.return_value.choices = [MagicMock(message=MagicMock(content="ok"))] + with ( + patch("gooddata_eval.core.agentic.alert_skill._OpenAI", mock_openai), + patch.dict("os.environ", {"OPENAI_API_KEY": "test-key"}), + ): + generate_simulated_alert_response("Which metric?", _normalize_expected_output(expected_output), []) + kwargs = mock_openai.return_value.chat.completions.create.call_args.kwargs + return kwargs["messages"][0]["content"], kwargs + + def test_to_number_int(): assert _to_number("42") == 42 @@ -51,6 +70,156 @@ 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_sim_user_states_always_trigger_proactively(): + # QA-28623: an expected-ALWAYS trigger used to be left unsaid, so the agent aligned + # trigger_interval with the date granularity and the sim-user accepted ONCE_PER_INTERVAL. + prompt, _ = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + assert "EVERY TIME" in prompt + assert "do not set any trigger interval" in prompt + + +def test_sim_user_states_once_trigger_proactively(): + prompt, _ = _sim_user_prompt({"Operator": "LESS_THAN", "Trigger": "One time"}) + assert "ONLY THE FIRST TIME" in prompt + + +def test_sim_user_silent_on_trigger_when_fixture_omits_it(): + # No Trigger key -> the case is meant to exercise the product default; stating one would + # turn it into a different test. + prompt, _ = _sim_user_prompt({"Operator": "GREATER_THAN", "Threshold": 100}) + assert "EVERY TIME" not in prompt + assert "trigger to" not in prompt + + +def test_sim_user_silent_on_trigger_for_anomaly(): + # ANOMALY legitimately defaults to ONCE_PER_INTERVAL and _check_trigger skips it. + prompt, _ = _sim_user_prompt({"Operator": "ANOMALY", "Trigger": "ONCE_PER_INTERVAL"}) + assert "EVERY TIME" not in prompt + + +def test_sim_user_refuses_extra_filters_when_none_expected(): + prompt, _ = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + assert "NO filters at all" in prompt + assert "All time" in prompt + + +def test_sim_user_refuses_extra_filters_beyond_expected_list(): + filters = [{"positiveAttributeFilter": {"label": {"identifier": {"id": "product_category"}}}}] + prompt, _ = _sim_user_prompt({"Operator": "LESS_THAN", "Trigger": "Every time", "Filters": filters}) + assert "ONLY filters this alert may have" in prompt + assert "product_category" in prompt + + +def test_sim_user_verifies_trigger_and_filters_at_confirmation(): + # Regression guard for QA-28113: rule 3 must not shrink back to recipients-only. + prompt, _ = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + assert "recipients, the trigger AND the filters" in prompt + + +def test_sim_user_rules_are_numbered_without_gaps(): + prompt, _ = _sim_user_prompt({"Operator": "ANOMALY", "Trigger": "ONCE_PER_INTERVAL"}) + numbers = [int(line.split(".", 1)[0]) for line in prompt.splitlines() if line[:1].isdigit()] + assert numbers == list(range(1, len(numbers) + 1)) + + +def test_sim_user_uses_deterministic_temperature(): + _, kwargs = _sim_user_prompt({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + assert kwargs["temperature"] == 0 + + +def test_score_comments_expose_trigger_and_filters(): + # QA-28623: a 0.0 score alone forced triage through CI logs / nested trace observations. + expected = _normalize_expected_output( + {"Operator": "GREATER_THAN", "Trigger": "Every time", "Threshold": 5000, "Metric": "Total Discounts (td)"} + ) + actual = { + "operator": "GREATER_THAN", + "threshold": 5000, + "trigger": "ONCE_PER_INTERVAL", + "trigger_interval": "DAY", + "filters": [{"relativeDateFilter": {"granularity": "DAY"}}], + "metric_id": "td", + "external_recipients": '["admin@gooddata.com"]', + } + comments = _score_comments(expected, actual, alert_created=True) + assert "'ALWAYS'" in comments["trigger_correct"] + assert "'ONCE_PER_INTERVAL/DAY'" in comments["trigger_correct"] + assert "relativeDateFilter" in comments["filters_correct"] + assert "admin@gooddata.com" in comments["recipients_correct"] + + +def test_score_comments_range_operator_reports_bounds(): + expected = _normalize_expected_output( + {"Operator": "BETWEEN", "threshold_from": 50000, "threshold_to": 200000, "Trigger": "Every time"} + ) + comments = _score_comments(expected, {"from_value": 50000, "to_value": 1}, alert_created=True) + assert "50000..200000" in comments["threshold_correct"] + assert "50000..1" in comments["threshold_correct"] + + +def test_score_comments_when_alert_never_created(): + expected = _normalize_expected_output({"Operator": "GREATER_THAN", "Trigger": "Every time"}) + comments = _score_comments(expected, {}, alert_created=False) + assert comments == {"alert_created": "create_metric_alert was never called"} + + +def test_evaluate_attaches_score_comments_to_the_trace(): + # Covers the Langfuse branch, which needs both a client and a dataset_item_id to run -- + # the other tests stop at run_agentic_alert_skill and never enter it. + fake_langfuse = MagicMock() + mock_client = MagicMock() + mock_client.send_message.return_value = ChatResult.model_validate( + { + "text_response": "Created the alert", + "created_visualizations": None, + "tool_call_events": [ + { + "functionName": "create_metric_alert", + "functionArguments": json.dumps( + { + "operator": "GREATER_THAN", + "threshold": 5000, + "trigger": "ONCE_PER_INTERVAL", + "trigger_interval": "DAY", + "filters": [{"relativeDateFilter": {"granularity": "DAY"}}], + } + ), + "result": "{}", + } + ], + "reasoning_step_count": 1, + } + ) + trace = MagicMock(id="trace-1", latency=1.0, total_cost=0.01) + + with ( + patch("gooddata_eval.core.agentic.alert_skill.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.alert_skill.GoodDataSdk"), + patch("gooddata_eval.core.agentic._langfuse.build_run_context", return_value=("run-name", {})), + patch( + "gooddata_eval.core.agentic._langfuse.find_traces_per_conversation", + return_value={"existing-conv": trace}, + ), + pytest.raises(AlertSkillAssertionError), + ): + evaluate_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Alert me when discounts get out of hand", + expected_output={"Operator": "GREATER_THAN", "Threshold": 5000, "Trigger": "Every time"}, + k=1, + max_iterations=1, + initial_conversation_id="existing-conv", + langfuse=fake_langfuse, + dataset_item_id="item-1", + ) + + comments = {call.kwargs["name"]: call.kwargs.get("comment") for call in fake_langfuse.create_score.call_args_list} + assert "ONCE_PER_INTERVAL/DAY" in comments["trigger_correct"] + assert "relativeDateFilter" in comments["filters_correct"] + + def test_alert_evaluation_strict_pass(): ev = AlertEvaluation( alert_created=True,