diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_clarification.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_clarification.py index 86a23b375..5e0a03b28 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_clarification.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_clarification.py @@ -370,7 +370,8 @@ def render(self) -> str: return "\n\n".join(f"**{text}**\n\n{answer}" for _, text, answer in sorted(sections, key=lambda s: s[0])) -def _all_questions(chat_result: ChatResult) -> list[dict]: +def all_questions(chat_result: ChatResult) -> list[dict]: + """Every question of every ``clarifyingQuestions`` part the reply carried, in order, whatever item it belongs to.""" parts = [p for p in chat_result.unhandled_parts or [] if p.get("type") == "clarifyingQuestions"] return [q for p in parts for q in p.get("questions") or []] @@ -502,7 +503,7 @@ def after_answers_mismatches( if skipped_questions_not_reasked: for question in skipped: asked_again = any( - _same_question(later, question) for _, reply, _ in replies_after for later in _all_questions(reply) + _same_question(later, question) for _, reply, _ in replies_after for later in all_questions(reply) ) if asked_again: out.append(f"asked the skipped question {question['text']!r} again") diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py index 2fafc30ba..6fe2a304c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py @@ -10,6 +10,8 @@ import jsonpatch +from gooddata_eval.core.agentic._clarification import all_questions +from gooddata_eval.core.agentic._conversation_context import CONFIRMATION_REPLY, asks_in_text, classify_reply, clip from gooddata_eval.core.agentic._failed_runs import build_failed_runs from gooddata_eval.core.agentic._gate import ( DEFAULT_GATE, @@ -54,9 +56,10 @@ # the loop ends there rather than replying into silence. Fewer than metric_skill's seven # because those rounds do carry new content, its reply being generated per turn by an LLM. # -# Only creation ever reaches the later rounds. An edit stops after the first turn, since the -# creation reply names charts and a date range and answers nothing a rename or a resize could -# have asked. +# Only creation ever reaches the later rounds. The creation reply names charts and a date range +# and answers nothing a rename or a resize could have asked, so an edit that asks back is +# answered once with its fixture's `clarification_reply`, and stops there when it has none or +# asks again. An edit that only asks to go ahead is told yes, which spends no reply. # # The cost of the slack is that four turns at ChatClient's 300s read timeout exceed the 720s # per-test timeout gdc-nas derives for a k=1 dataset, so a run that stalls on every turn is cut @@ -1067,9 +1070,14 @@ def build_simulated_reply(expected_output: dict) -> str: """Build the reply the simulated user sends when the agent asks back instead of drafting. Deterministic and LLM-free: the only things the agent still needs are the charts and the - date range, and both are already in the expectation. A fixed reply also keeps a failure - attributable to the agent rather than to a simulated user that phrased things differently - each run. + date range, and the expectation states both whenever the case checks them. A fixed reply + also keeps a failure attributable to the agent rather than to a simulated user that + phrased things differently each run. + + The date range is said only when the case states one, ``null`` included. A case without + the key does not look at the date filter, so naming a period would be the reply asserting + something the case never did -- and "all time" can contradict the date filters of the + existing charts the case expects to be reused. """ visualizations = expected_output.get("visualizations") or [] existing = [str(v.get("title", "")) for v in visualizations if v.get("id") is not None] @@ -1095,11 +1103,16 @@ def build_simulated_reply(expected_output: dict) -> str: quoted = " and ".join(f'"{c}"' for c in charts) placements.append(f"{quoted} on {on_tab}" if charts else on_tab) tabs = f"Use these tabs: {'; '.join(placements)}. " if placements else "" + date = ( + f"Date range: {_date_range_text(expected_output.get('date_range'))}. " + if _has_date_range(expected_output) + else "" + ) return ( f"Please use these charts: {', and '.join(segments)}. " f"{tabs}" - f"Date range: {_date_range_text(expected_output.get('date_range'))}. " + f"{date}" f"Anything else is up to you. Please create the dashboard now." ) @@ -1243,6 +1256,16 @@ def _validate_expectation(expected_output: dict) -> None: for pattern in must_not_match: _compile_pattern(pattern) + if "clarification_reply" in expected_output: + reply = expected_output["clarification_reply"] + if not isinstance(reply, str) or not reply.strip(): + raise ValueError(f"clarification_reply must be a non-empty reply, got {reply!r}") + if not _is_edit(expected_output): + raise ValueError( + "clarification_reply is for an edit case; a creation case is answered with the reply " + "built from its charts and date range" + ) + forbidden = _forbidden_visualizations(expected_output) if not isinstance(forbidden, list): raise ValueError(f"must_not_contain must be a list of chart expectations, got {forbidden!r}") @@ -2002,6 +2025,47 @@ def evaluate_dashboard_response( ) +# Long enough for a question with its options, short enough to keep the assertion message readable. +_QUOTED_QUESTION_CHARS = 300 + + +def _asked_question(chat_result: ChatResult, response_text: str) -> str: + """What the agent asked: the ``clarifyingQuestions`` part's questions when it sent one, else its text. + + The structured part is what ``classify_reply`` reads as the question, and prose sent beside it + ("I need more details.") would quote the lead-in rather than what was asked. + """ + questions = [str(q.get("text") or "").strip() for q in all_questions(chat_result)] + return " ".join(q for q in questions if q) or response_text + + +def _quote(text: str) -> str: + """The agent's words cut to a readable length and quoted, or a marker when it said nothing.""" + clipped = clip(text, _QUOTED_QUESTION_CHARS) + return repr(clipped) if clipped else "(no text)" + + +def _unacted_edit_note(response_text: str) -> str: + """Say what an edit run stopped on when the agent neither patched nor asked. + + Not scored, like ``_unanswered_edit_note``: it tells a reply that gave up apart from one that asked. + """ + return f"the agent replied on an edit without a patch or a question, so the run stopped: {_quote(response_text)}" + + +def _unanswered_edit_note(question: str, *, replied: bool, has_reply: bool) -> str: + """Say which question an edit run stopped on, so triage can tell a fixture gap from the agent. + + Not scored: the run already fails on the missing patch, and this only says why it never got one. + """ + quoted = _quote(question) + if replied: + return f"the agent asked again after the case's clarification_reply, and the run stopped: {quoted}" + if has_reply: + return f"the turn budget ran out before the case's clarification_reply could be sent: {quoted}" + return f"the agent asked back on an edit and the case has no clarification_reply, so the run stopped: {quoted}" + + def _execute_single_dashboard_run( client: ChatClient, conversation_id: str, @@ -2024,6 +2088,11 @@ def _execute_single_dashboard_run( turns = 0 steps = 0 current_question = question + # Validated to be on an edit case only, and a non-empty string when present. + clarification_reply: str | None = expected_output.get("clarification_reply") + reply_sent = False + # Why an edit stopped without a patch, appended to the evaluation as an unscored note. + edit_note: str | None = None reasoning_steps: list[str] = [] response_id: str | None = None all_tool_call_events: list[ToolCallEvent] = [] @@ -2033,9 +2102,7 @@ def _execute_single_dashboard_run( tool_index_offset = 0 reasoning_index_offset = 0 # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that - # simply runs out of range() (or the is_edit "no reply of its own" branch below, which - # is the same shape of outcome -- the agent answered, just not with the dashboard tool) - # is labelled correctly with no trailing else. + # simply runs out of range() is labelled correctly with no trailing else. exit_reason = LoopExit.BUDGET_EXHAUSTED run_started = time.monotonic() @@ -2121,36 +2188,77 @@ def _execute_single_dashboard_run( # restates the charts and the date range and asks again for the dashboard, which is # pressure to build the very thing the case says must not be built -- so a correct # agent would be talked out of the right answer by the harness. + exit_reason = LoopExit.STOPPED_EARLY break + reply_kind = classify_reply(chat_result, response_text) + if is_edit and reply_kind == "confirmation" and asks_in_text(chat_result, response_text): + # "Which widget should I resize? Please confirm." asks something a yes does not answer. + reply_kind = "question" + if is_edit: + # The creation reply names charts and a date range, which answers nothing a rename + # or a resize could have asked, and nothing here can guess what an edit question + # needs. Only the fixture's own reply is sent, only to a turn that asked (as + # document_skill gates it), and only once: a second question is one its author did + # not foresee, so the run stops and says what was asked, which tells a fixture that + # needs a reply apart from an agent that did the wrong thing. A turn that asked + # nothing -- a rejected patch reported back, say -- is not nudged into a retry. A turn + # that only asks to go ahead is told yes, as conversation.py does; that spends no reply, + # and the iteration cap still bounds it. + # Checked before the iteration cap: these stops are the harness having nothing to + # send, not the budget running out, and each still says what went unanswered. + if reply_kind == "no_action": + edit_note = _unacted_edit_note(response_text) + exit_reason = LoopExit.STOPPED_EARLY + break + if reply_kind == "question" and (clarification_reply is None or reply_sent): + edit_note = _unanswered_edit_note( + _asked_question(chat_result, response_text), + replied=reply_sent, + has_reply=clarification_reply is not None, + ) + exit_reason = LoopExit.STOPPED_EARLY + break if iteration >= max_iterations - 1: + if is_edit and reply_kind == "question": + # A question here still had its reply to send; a confirmation needs no note. + edit_note = _unanswered_edit_note( + _asked_question(chat_result, response_text), replied=False, has_reply=True + ) break + if is_edit and reply_kind == "confirmation": + current_question = CONFIRMATION_REPLY + answering_with = "a confirmation" + elif is_edit and clarification_reply is not None: + reply_sent = True + current_question = clarification_reply + answering_with = "the case's clarification_reply" + else: + current_question = build_simulated_reply(expected_output) + answering_with = "the expected charts and date range" log_timer( f"[timer] dashboard_skill {conversation_id} GoodData turn {turns} complete after " - f"{agent_elapsed:.2f}s; answering with the expected charts and date range" + f"{agent_elapsed:.2f}s; answering with {answering_with}" ) - if is_edit: - # The creation reply names charts and a date range, which answers nothing a rename - # or a resize could have asked. Rather than send something the question did not - # ask for, stop: the failure is the signal that an edit case needs a reply of its - # own, and inventing one here would hide which cases actually need it. - break - current_question = build_simulated_reply(expected_output) + + evaluation = evaluate_dashboard_response( + tool_result, + dashboard_part, + expected_output, + _skill_activated(all_tool_call_events, _required_skill(expected_output)), + patch_part=patch_part, + answer_text=answer_text, + # Any call, not just a successful one -- see DashboardEvaluation. BOTH tools, + # not the one this case selected: a refusal is a creation shape, so `tool` is + # `draft_dashboard`, and an agent that routed to the editor and had a + # `patch_dashboard` call rejected would otherwise leave this false and pass. + producing_tool_called=any(tc.function_name in (_DRAFT_TOOL, _PATCH_TOOL) for tc in all_tool_call_events), + ) + if edit_note is not None: + evaluation.notes.append(edit_note) return DashboardRunResult( conversation_id=conversation_id, - evaluation=evaluate_dashboard_response( - tool_result, - dashboard_part, - expected_output, - _skill_activated(all_tool_call_events, _required_skill(expected_output)), - patch_part=patch_part, - answer_text=answer_text, - # Any call, not just a successful one -- see DashboardEvaluation. BOTH tools, - # not the one this case selected: a refusal is a creation shape, so `tool` is - # `draft_dashboard`, and an agent that routed to the editor and had a - # `patch_dashboard` call rejected would otherwise leave this false and pass. - producing_tool_called=any(tc.function_name in (_DRAFT_TOOL, _PATCH_TOOL) for tc in all_tool_call_events), - ), + evaluation=evaluation, tool_result=tool_result, dashboard_part=dashboard_part, patch_part=patch_part, diff --git a/packages/gooddata-eval/src/gooddata_eval/core/models.py b/packages/gooddata-eval/src/gooddata_eval/core/models.py index 435ac811a..094cee08e 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/models.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/models.py @@ -40,6 +40,12 @@ class LoopExit(str, Enum): """The loop hit ``max_iterations`` without the agent producing its output. Says nothing about whether the agent was on track; it may have been one turn away.""" + STOPPED_EARLY = "stopped_early" + """The agent replied and the harness had nothing to send back, so the loop stopped with + turns left: a question the case has no reply for, a second question after the case's + reply was spent, a reply that neither asked nor acted, or a refusal case answered in one + turn, where any follow-up would press the agent to build what it must not.""" + SIMULATED_USER_FAILED = "simulated_user_failed" """The harness's own simulated-user model failed to produce a follow-up. A harness-side fault, not an agent one -- reporting should treat it as an error, not a scored failure.""" diff --git a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py index 2edfc58e8..e7d577df2 100644 --- a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock, patch import pytest +from gooddata_eval.core.agentic._conversation_context import CONFIRMATION_REPLY from gooddata_eval.core.agentic.dashboard_skill import ( DashboardEvaluation, DashboardSkillAssertionError, @@ -718,8 +719,8 @@ def test_a_patch_that_does_not_apply_claims_nothing_about_charts(self): def test_an_edit_that_asks_back_stops_rather_than_sending_the_creation_reply(self): """The creation reply names charts and a date range, which answers nothing a rename or - a resize could have asked. Sending it anyway would hide which cases need a reply of - their own.""" + a resize could have asked. Without a reply of its own the run stops, and says what the + agent asked so triage sees the case needs one.""" client = MagicMock() client.send_message.return_value = _chat_result( tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], @@ -729,6 +730,180 @@ def test_an_edit_that_asks_back_stops_rather_than_sending_the_creation_reply(sel assert client.send_message.call_count == 1 assert summary.best.total_turns == 1 assert not summary.pass_at_k + # Three turns were left: the run stopped because nothing could answer, not on the budget. + assert summary.best.exit_reason is LoopExit.STOPPED_EARLY + assert summary.best.evaluation.notes == [ + "the agent asked back on an edit and the case has no clarification_reply, so the run stopped: " + "'Which widget did you mean?'" + ] + + def test_an_edit_that_asks_back_is_answered_with_its_clarification_reply(self) -> None: + reply = "Rename the Order status chart. Apply it now, no need to confirm." + client = MagicMock() + client.send_message.side_effect = [ + _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Which widget did you mean?", + ), + _chat_result( + tool_calls=[_tool_call("patch_dashboard", _patch_result())], + parts=[_edit_base_part(), _patch_part(TestPreservedFilters._RENAME, visualizations=[_ORDER_STATUS])], + ), + ] + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": reply}, max_iterations=None) + assert [call.args[1] for call in client.send_message.call_args_list] == [ + "Could you create a dashboard?", + reply, + ] + assert summary.pass_at_k + assert summary.best.total_turns == 2 + assert summary.best.evaluation.notes == [] + + def test_the_clarification_reply_is_sent_once_and_a_second_question_is_quoted(self) -> None: + """A second question is one the fixture's author did not foresee. Repeating the same + reply would not answer it, so the run stops and names it.""" + reply = "Add both at the end of the section. Apply it now, no need to confirm." + client = MagicMock() + client.send_message.side_effect = [ + _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Where should I place them?", + ), + _chat_result(text="Should the new chart be full width?"), + ] + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": reply}, max_iterations=None) + assert client.send_message.call_count == 2 + assert not summary.pass_at_k + assert summary.best.exit_reason is LoopExit.STOPPED_EARLY + assert summary.best.evaluation.notes == [ + "the agent asked again after the case's clarification_reply, and the run stopped: " + "'Should the new chart be full width?'" + ] + + def test_an_edit_turn_that_asks_nothing_is_not_sent_the_clarification_reply(self) -> None: + """A turn that reports a rejected patch asked nothing; sending the reply would nudge a retry.""" + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="I could not apply the change to the dashboard.", + ) + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": "Apply it now."}, max_iterations=None) + assert client.send_message.call_count == 1 + assert not summary.pass_at_k + assert summary.best.exit_reason is LoopExit.STOPPED_EARLY + assert summary.best.evaluation.notes == [ + "the agent replied on an edit without a patch or a question, so the run stopped: " + "'I could not apply the change to the dashboard.'" + ] + + def test_an_edit_that_asks_to_go_ahead_is_told_yes_without_spending_its_reply(self) -> None: + """A confirmation asks nothing the fixture has to answer: "Yes, go ahead." settles it, and the + clarification_reply stays for a real question.""" + reply = "Add both at the end of the section. Apply it now, no need to confirm." + client = MagicMock() + client.send_message.side_effect = [ + _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="I will rename the chart. Shall I apply this?", + ), + _chat_result(text="Where should I place them?"), + _chat_result( + tool_calls=[_tool_call("patch_dashboard", _patch_result())], + parts=[_edit_base_part(), _patch_part(TestPreservedFilters._RENAME, visualizations=[_ORDER_STATUS])], + ), + ] + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": reply}, max_iterations=None) + assert [call.args[1] for call in client.send_message.call_args_list] == [ + "Could you create a dashboard?", + CONFIRMATION_REPLY, + reply, + ] + assert summary.pass_at_k + assert summary.best.evaluation.notes == [] + + def test_a_question_asked_beside_a_confirmation_is_sent_the_clarification_reply(self) -> None: + """A real question followed by "Please confirm" is still a question, so the fixture's reply answers it.""" + reply = "Resize the Order status chart. Apply it now, no need to confirm." + client = MagicMock() + client.send_message.side_effect = [ + _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Which widget should I resize? Please confirm.", + ), + _chat_result( + tool_calls=[_tool_call("patch_dashboard", _patch_result())], + parts=[_edit_base_part(), _patch_part(TestPreservedFilters._RENAME, visualizations=[_ORDER_STATUS])], + ), + ] + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": reply}, max_iterations=None) + assert [call.args[1] for call in client.send_message.call_args_list] == [ + "Could you create a dashboard?", + reply, + ] + assert summary.pass_at_k + + def test_a_cap_reached_before_the_clarification_reply_is_sent_still_quotes_the_question(self) -> None: + """With one turn there is no room for the reply: the budget ended the run, and the note says so.""" + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Where should I place them?", + ) + summary = _run_with(client, {**_DE01_EXPECTED, "clarification_reply": "At the end."}, max_iterations=1) + assert client.send_message.call_count == 1 + assert summary.best.exit_reason is LoopExit.BUDGET_EXHAUSTED + assert summary.best.evaluation.notes == [ + "the turn budget ran out before the case's clarification_reply could be sent: 'Where should I place them?'" + ] + + def test_a_structured_question_is_quoted_rather_than_the_text_beside_it(self) -> None: + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + parts=[ + { + "type": "clarifyingQuestions", + "questions": [{"text": "Which widget?", "control": {"options": []}}], + } + ], + text="I need more details.", + ) + summary = _run_with(client, _DE01_EXPECTED, max_iterations=None) + assert summary.best.evaluation.notes == [ + "the agent asked back on an edit and the case has no clarification_reply, so the run stopped: " + "'Which widget?'" + ] + + def test_a_long_unanswered_question_is_cut_in_the_note(self) -> None: + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Which option? " + "Option " * 100, + ) + note = _run_with(client, _DE01_EXPECTED, max_iterations=None).best.evaluation.notes[0] + assert note.endswith("…'") + assert len(note) < 450 + + @pytest.mark.parametrize( + ("reply", "message"), + [ + ("", "clarification_reply must be a non-empty reply"), + (" ", "clarification_reply must be a non-empty reply"), + (["Add it at the end."], "clarification_reply must be a non-empty reply"), + ], + ) + def test_an_unusable_clarification_reply_is_rejected_up_front(self, reply: object, message: str) -> None: + client = MagicMock() + with pytest.raises(ValueError, match=message): + _run_with(client, {**_DE01_EXPECTED, "clarification_reply": reply}) + client.send_message.assert_not_called() + + def test_a_creation_case_cannot_carry_a_clarification_reply(self) -> None: + """A creation case is answered with the reply built from its charts and date range.""" + client = MagicMock() + with pytest.raises(ValueError, match="clarification_reply is for an edit case"): + _run_with(client, {**_DC05_EXPECTED, "clarification_reply": "Use these charts."}) + client.send_message.assert_not_called() def test_a_creation_case_still_gets_its_clarifying_round(self): """The same cap that an edit never reaches is still spent on creation.""" @@ -937,6 +1112,19 @@ def test_an_authored_chart_is_appended(self): "Anything else is up to you. Please create the dashboard now." ) + def test_a_case_without_a_date_range_says_no_period(self) -> None: + """The case does not look at the date filter, so the reply must not pick one -- "all time" + could contradict the date filter of an existing chart the case expects to be reused.""" + expected = {k: v for k, v in _DC03_EXPECTED.items() if k != "date_range"} + assert build_simulated_reply(expected) == ( + 'Please use these charts: Activity by Hour, and a new chart titled "Net Orders by Customer Age". ' + "Anything else is up to you. Please create the dashboard now." + ) + + def test_a_null_date_range_still_asks_for_all_time(self) -> None: + """Null is a stated range: the case requires all time and checks it.""" + assert "Date range: all time. " in build_simulated_reply({**_DC03_EXPECTED, "date_range": None}) + def test_an_authored_chart_alone_reads_naturally(self): expected = {"visualizations": [{"id": None, "title": "Net Orders by Customer Age"}], "date_range": None} assert 'Please use these charts: a new chart titled "Net Orders by Customer Age".' in build_simulated_reply( @@ -2902,3 +3090,5 @@ def test_the_simulated_user_never_argues_a_refusal_into_building(self): summary = _run_with(client, self._NO_DATA) assert client.send_message.call_count == 1 assert summary.best.evaluation.strict_pass + # Turns were left: the harness chose not to reply, so the budget did not end it. + assert summary.best.exit_reason is LoopExit.STOPPED_EARLY