diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py index f621afa27..9f42faa92 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/kda_skill.py @@ -5,7 +5,6 @@ import logging import os -import re from dataclasses import dataclass from gooddata_eval.core.chat.sse_client import ChatClient @@ -15,36 +14,79 @@ _log = logging.getLogger(__name__) _DEFAULT_K = 1 -# Disambiguation safety net only (create+execute always run together in the same -# turn) -- 3 covers metric and period each needing their own clarifying question. -_DEFAULT_MAX_ITERATIONS = 3 +# Disambiguation safety net only (create+execute always run together in the same turn) -- +# 3 real questions' worth (metric, period, +1 slack) since a simulated reply is now sent on +# every non-final turn (see run_agentic_kda_skill), not just ones classified as a question. +_DEFAULT_MAX_ITERATIONS = 4 -def _is_asking_kda_clarification(text: str) -> bool: - """True if ``text`` reads as the agent asking for input, not a final answer. - - KDA-specific, not shared with metric_skill.py/conversation.py -- each skill's - disambiguation heuristic has already drifted independently. Requires the text to - end on "?" (a "?" anywhere also matches a final answer that merely quotes one). +def _build_period_hint(expected_output: dict) -> str | None: + """Build a period hint from whichever of expected_output's Date Attribute/Analyzed + Period/Reference Period are present -- a question about only one of them (e.g. "which + date dimension?") must still get an answerable hint, not None just because the other + two are absent. + """ + date_attr = expected_output.get("Date Attribute") + analyzed = expected_output.get("Analyzed Period") + reference_period = expected_output.get("Reference Period") + if not (date_attr or analyzed or reference_period): + return None + parts = [] + if date_attr: + parts.append(date_attr) + if analyzed and reference_period: + parts.append(f"comparing {analyzed} to {reference_period}") + elif analyzed: + parts.append(f"period {analyzed}") + elif reference_period: + parts.append(f"compared to {reference_period}") + return ", ".join(parts) + + +def _build_clarification_prompt( + agent_message: str, measure_candidates: dict | list[dict] | None, period_hint: str | None +) -> str: + """Build the simulated-user prompt, referencing only whatever candidates/period-hint + are actually usable -- an empty/None candidate must drop the "acceptable metric/fact" + clause entirely rather than assert a literal "None" as if it were a real option. """ - if not text: - return False - t = text.strip().lower() - if t.endswith("?"): - return True - # "To clarify, ..." means "in other words" (a final answer), not a request for one -- - # strip it first so "clarif" below only matches genuine clarification requests. - t = re.sub(r"^(just )?to clarify,?\s*", "", t) - return "could you" in t or "please provide" in t or "clarif" in t - - -def generate_simulated_kda_response(agent_message: str, measure_candidates: dict | list[dict] | None) -> str: + candidates = [ + c for c in (measure_candidates if isinstance(measure_candidates, list) else [measure_candidates]) if c + ] + reference = "" + if candidates: + candidate_desc = "; or ".join( + f"{c.get('type')} '{c.get('id')}'" + (f" (aggregation {c['aggregation']})" if c.get("aggregation") else "") + for c in candidates + ) + reference = f"an acceptable metric/fact is {candidate_desc}" + if period_hint: + reference = ( + f"{reference}; the intended time period is {period_hint}" + if reference + else f"the intended time period is {period_hint}" + ) + return ( + f"You are simulating a user in a conversation with a BI assistant that runs key driver " + f"analysis. The assistant asked: '{agent_message}'. " + + (f"For reference, {reference}. " if reference else "") + + "Reply briefly as the user, answering whichever of those the assistant actually asked about." + ) + + +def generate_simulated_kda_response( + agent_message: str, + measure_candidates: dict | list[dict] | None, + period_hint: str | None = None, +) -> str: """Generate a user reply to keep the KDA-skill conversation going (gpt-4o-mini). - Used only when the agent asks a clarifying question instead of triggering KDA - directly. Picks *any* candidate from ``measure_candidates`` -- scope only needs KDA - to trigger, not the resulting measure to be exactly right. Always OpenAI regardless - of the combo's own provider -- this is test-harness plumbing, not the system under test. + Called on any turn that didn't trigger KDA, whatever the agent's response actually + said -- most often a clarifying question about the measure, the period, or both, so + both are given as reference and the reply answers whichever was actually asked. + Scope only needs KDA to trigger, not the resulting measure/period to be exactly + right. Always OpenAI regardless of the combo's own provider -- this is + test-harness plumbing, not the system under test. """ try: from openai import OpenAI # noqa: PLC0415 @@ -56,17 +98,7 @@ def generate_simulated_kda_response(agent_message: str, measure_candidates: dict raise OSError("OPENAI_API_KEY environment variable is not set") client = OpenAI(api_key=api_key) - candidates = measure_candidates if isinstance(measure_candidates, list) else [measure_candidates or {}] - candidate_desc = "; or ".join( - f"{c.get('type')} '{c.get('id')}'" + (f" (aggregation {c['aggregation']})" if c.get("aggregation") else "") - for c in candidates - ) - prompt = ( - f"You are simulating a user in a conversation with a BI assistant that runs key driver " - f"analysis. The assistant said: '{agent_message}'. " - f"The user is happy to proceed with any of the following: {candidate_desc}. " - f"Reply briefly as the user, picking whichever of those the assistant offered." - ) + prompt = _build_clarification_prompt(agent_message, measure_candidates, period_hint) response = client.chat.completions.create( model="gpt-4o-mini", messages=[{"role": "user", "content": prompt}], @@ -171,9 +203,12 @@ def run_agentic_kda_skill( Each run is normally one message, one turn -- create and execute are always called together in the same turn (the skill's own system prompt: "NO confirmation needed"). - The only thing that can extend a run up to ``max_iterations`` turns is the agent - asking a clarifying question instead of triggering KDA directly; a simulated user - reply nudges it forward. + A run only extends past turn 1, up to ``max_iterations``, when the agent's response + has no create call and isn't empty; a simulated user reply is then always sent, with + no attempt to classify whether the text was actually asking for input (matching + visualization.py/alert_skill.py's own break conditions) -- missing a genuine + clarifying question hard-fails the run, while sending one after an unrecognized final + answer only costs one harmless extra turn, so the asymmetry favors never guessing. """ if k < 1: # k=0 or negative would otherwise silently run once, indistinguishable from k=1. @@ -212,17 +247,22 @@ def _run_once(conv_id: str) -> KdaRunResult: # execute tool isn't available at all when data-sharing is off for the org). turn_wall_clock_sec = chat_result.turn_wall_clock_sec break + if not response_text: + break if iteration >= max_iterations - 1: break - if _is_asking_kda_clarification(response_text): - measure_candidates = expected_output.get("Measure") if isinstance(expected_output, dict) else None - try: - current_question = generate_simulated_kda_response(response_text, measure_candidates) - disambiguated = True - except Exception as exc: # noqa: BLE001 -- safety net, not the assertion; end only this run - _log.warning("Simulated KDA user reply failed for conversation %s: %s", conv_id, exc) - break - else: + # No text classification -- matches visualization.py/alert_skill.py: break only on + # the goal signal (create_args set) or an empty response, otherwise always send a + # simulated reply. A false positive (agent had already given a final answer) costs + # one harmless extra turn; a false negative (missing a genuine clarifying question) + # would hard-fail the run, so the asymmetry favors never trying to tell them apart. + measure_candidates = expected_output.get("Measure") if isinstance(expected_output, dict) else None + period_hint = _build_period_hint(expected_output) if isinstance(expected_output, dict) else None + try: + current_question = generate_simulated_kda_response(response_text, measure_candidates, period_hint) + disambiguated = True + except Exception as exc: # noqa: BLE001 -- safety net, not the assertion; end only this run + _log.warning("Simulated KDA user reply failed for conversation %s: %s", conv_id, exc) break ev = _evaluate_run(create_args, execute_result, turn_completed, disambiguated) diff --git a/packages/gooddata-eval/tests/test_agentic_kda_skill.py b/packages/gooddata-eval/tests/test_agentic_kda_skill.py index d8fa2cdfb..742f4c2f5 100644 --- a/packages/gooddata-eval/tests/test_agentic_kda_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_kda_skill.py @@ -8,9 +8,10 @@ from gooddata_eval.core.agentic.kda_skill import ( KdaEvaluation, KdaSkillAssertionError, + _build_clarification_prompt, + _build_period_hint, _evaluate_run, _extract_kda_calls, - _is_asking_kda_clarification, evaluate_agentic_kda_skill, run_agentic_kda_skill, ) @@ -67,51 +68,59 @@ def _no_kda_chat_result( # --------------------------------------------------------------------------- # -# _is_asking_kda_clarification +# _build_clarification_prompt # --------------------------------------------------------------------------- # -@pytest.mark.parametrize( - "text", - ["Could you clarify which metric?", "Please provide the date range.", "Did you mean revenue?"], -) -def test_is_asking_kda_clarification_true(text): - assert _is_asking_kda_clarification(text) is True +def test_build_clarification_prompt_omits_reference_clause_when_no_candidates_or_period(): + # Regression (chi My's review): with no usable candidates, the old code still asserted + # "an acceptable metric/fact is None 'None'" as if it were a real option -- likely to + # make the simulated user invent a metric literally named "None". No candidates and no + # period hint must drop the whole "For reference, ..." clause instead. + prompt = _build_clarification_prompt("Which date range?", None, None) + assert "None" not in prompt + assert "For reference" not in prompt -def test_is_asking_kda_clarification_false_on_plain_statement(): - assert _is_asking_kda_clarification("Here is the key driver analysis result.") is False +def test_build_clarification_prompt_includes_only_period_hint_when_no_candidates(): + prompt = _build_clarification_prompt("Which period?", None, "2026-2 vs 2026-1") + assert "None" not in prompt + assert "the intended time period is 2026-2 vs 2026-1" in prompt -def test_is_asking_kda_clarification_false_on_empty(): - assert _is_asking_kda_clarification("") is False +def test_build_clarification_prompt_includes_candidates_and_period_hint(): + prompt = _build_clarification_prompt( + "Which metric and period?", {"type": "metric", "id": "revenue"}, "2026-2 vs 2026-1" + ) + assert "an acceptable metric/fact is metric 'revenue'" in prompt + assert "the intended time period is 2026-2 vs 2026-1" in prompt -def test_is_asking_kda_clarification_false_when_question_mark_is_not_the_final_answer(): - # Regression guard for the original bug: a final answer that merely quotes or - # rhetorically references a question must not be mistaken for a clarifying question. - text = 'The user asked "what changed?" so here is the key driver breakdown they requested.' - assert _is_asking_kda_clarification(text) is False +# --------------------------------------------------------------------------- # +# _build_period_hint +# --------------------------------------------------------------------------- # +def test_build_period_hint_none_when_no_period_fields_present(): + assert _build_period_hint({"Measure": {"type": "metric", "id": "revenue"}}) is None -@pytest.mark.parametrize( - "text", - [ - "To clarify, revenue rose 12% quarter over quarter.", - "Just to clarify, the increase was driven by the South region.", - ], -) -def test_is_asking_kda_clarification_false_on_to_clarify_discourse_marker(text): - # Regression guard: "to clarify, ..." is a discourse marker ("in other words") that - # introduces a restated FINAL answer, not a request for one -- the bare "clarif" in t - # substring check would otherwise mistake this for a clarifying question and burn a - # simulated-reply turn on an answer that was already complete. - assert _is_asking_kda_clarification(text) is False +def test_build_period_hint_all_three_fields(): + hint = _build_period_hint( + {"Date Attribute": "transaction_date.quarter", "Analyzed Period": "2026-2", "Reference Period": "2026-1"} + ) + assert hint == "transaction_date.quarter, comparing 2026-2 to 2026-1" -def test_is_asking_kda_clarification_true_for_genuine_clarify_request_despite_marker_strip(): - # The discourse-marker strip must not eat a genuine request that happens to start the - # same way it's phrased in practice. No trailing "?" here specifically so this exercises - # the "could you" substring check post-strip, not the separate endswith("?") check. - assert _is_asking_kda_clarification("To clarify, could you tell me which region you mean") is True +def test_build_period_hint_date_attribute_only(): + # A dataset item carrying only Date Attribute (agent asks "which date dimension should + # I use?") must still get an answerable hint -- this used to require all 3 fields and + # reproduced the same gap the metric-clarification fix closed, just narrower. + assert _build_period_hint({"Date Attribute": "transaction_date.quarter"}) == "transaction_date.quarter" + + +def test_build_period_hint_analyzed_period_only(): + assert _build_period_hint({"Analyzed Period": "2026-2"}) == "period 2026-2" + + +def test_build_period_hint_reference_period_only(): + assert _build_period_hint({"Reference Period": "2026-1"}) == "compared to 2026-1" # --------------------------------------------------------------------------- # @@ -427,6 +436,128 @@ def test_run_agentic_kda_skill_marks_disambiguated_after_a_simulated_reply(): assert summary.best.evaluation.triggered is True +def test_run_agentic_kda_skill_disambiguates_on_question_followed_by_option_list(): + # Regression (QA-28800): the real captured response ends with a bullet list of + # candidate metrics. Before this module dropped text classification in favor of + # always retrying on a non-empty, non-triggering response (matching + # visualization.py/alert_skill.py), a heuristic that only matched "?" endings gave + # up after turn 1 (triggered=False) instead of ever nudging the simulated user to + # pick one. + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _no_kda_chat_result( + 'I found two different "Total Net Revenue" metrics in your data model. ' + "Which one should I analyze for the 2024 vs 2023 drop?\n\n" + "- {metric/metric_l1_sql_net_sales_summary_net_revenue}\n" + "- {metric/metric_l1_total_net_revenue}" + ), + _kda_chat_result(success=True), + ] + + with ( + patch("gooddata_eval.core.agentic.kda_skill.ChatClient", return_value=mock_client), + patch( + "gooddata_eval.core.agentic.kda_skill.generate_simulated_kda_response", + return_value="Use metric_l1_sql_net_sales_summary_net_revenue.", + ) as mock_simulate, + ): + summary = run_agentic_kda_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Why did Total Net Revenue of Net Sales Summary drop in 2024 compared to 2023?", + expected_output=_EXPECTED, + k=1, + max_iterations=2, + ) + + mock_simulate.assert_called_once() + assert summary.best.evaluation.disambiguated is True + assert summary.best.evaluation.triggered is True + assert mock_client.send_message.call_count == 2 + + +def test_run_agentic_kda_skill_retries_on_bold_markdown_option_list_with_no_space(): + # Regression (chi My's review): a prior classifier-based fix required a space right + # after the list marker, so "**Option 1**: revenue" (bold markdown, no space between + # the two asterisks) would have been misread as a final answer. Dropping content + # classification entirely (see run_agentic_kda_skill's docstring) makes this -- and any + # other future response shape -- a non-issue: a non-triggering, non-empty response + # always gets a simulated reply now, regardless of how it's formatted. + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _no_kda_chat_result("Which one should I analyze?\n**Option 1**: revenue\n**Option 2**: gross profit"), + _kda_chat_result(success=True), + ] + + with ( + patch("gooddata_eval.core.agentic.kda_skill.ChatClient", return_value=mock_client), + patch( + "gooddata_eval.core.agentic.kda_skill.generate_simulated_kda_response", + return_value="Use revenue.", + ) as mock_simulate, + ): + summary = run_agentic_kda_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Why did revenue drop?", + expected_output=_EXPECTED, + k=1, + max_iterations=2, + ) + + mock_simulate.assert_called_once() + assert summary.best.evaluation.disambiguated is True + assert summary.best.evaluation.triggered is True + + +def test_run_agentic_kda_skill_disambiguates_on_period_clarification(): + # generate_simulated_kda_response used to only know about measure candidates -- if the + # agent asked about the PERIOD instead, it had nothing period-specific to answer with. + # Verify the period hint built from expected_output's Date Attribute/Analyzed + # Period/Reference Period reaches the simulated-reply call. + expected_output = { + "Measure": {"type": "metric", "id": "revenue"}, + "Date Attribute": "transaction_date.quarter", + "Analyzed Period": "2026-2", + "Reference Period": "2026-1", + } + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _no_kda_chat_result("Which period would you like to compare?"), + _kda_chat_result(success=True), + ] + + with ( + patch("gooddata_eval.core.agentic.kda_skill.ChatClient", return_value=mock_client), + patch( + "gooddata_eval.core.agentic.kda_skill.generate_simulated_kda_response", + return_value="Compare 2026-2 to 2026-1.", + ) as mock_simulate, + ): + summary = run_agentic_kda_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Why did revenue drop?", + expected_output=expected_output, + k=1, + max_iterations=2, + ) + + mock_simulate.assert_called_once_with( + "Which period would you like to compare?", + {"type": "metric", "id": "revenue"}, + "transaction_date.quarter, comparing 2026-2 to 2026-1", + ) + assert summary.best.evaluation.disambiguated is True + assert summary.best.evaluation.triggered is True + + def test_run_agentic_kda_skill_disambiguates_when_expected_output_is_not_a_dict(): # DatasetItem.expected_output on the gdc-nas side allows str/list, not just dict. # expected_output.get("Measure") would raise AttributeError on those shapes, silently @@ -457,7 +588,7 @@ def test_run_agentic_kda_skill_disambiguates_when_expected_output_is_not_a_dict( max_iterations=2, ) - mock_generate.assert_called_once_with("Could you clarify which measure?", None) + mock_generate.assert_called_once_with("Could you clarify which measure?", None, None) assert summary.best.evaluation.disambiguated is True assert summary.best.evaluation.triggered is True