From c9a9100d93444e15ae37de0e6cc6bf80a44fbfc0 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 9 Sep 2026 12:13:03 +0200 Subject: [PATCH 1/3] feat(gooddata-eval): record why an agentic simulated-user loop stopped Every agentic evaluator drives a loop that can exit several ways, but the result only ever recorded *whether* the agent produced its output. A run that ran out of turns while doing the right thing was reported identically to one that refused, and identically to one that answered wrongly. It is worse than a missing field, because the downstream checks are all of the form `produced_output and `. An exhausted alert run reports operator_correct, threshold_correct, metric_correct and recipients_correct as False -- four specific-sounding content failures for work the agent was never given the chance to attempt. Adds `LoopExit` (core/models.py) and threads it through all five loops, plus `turns_used`/`max_iterations` in `detail`: success the agent produced its output agent_silent neither text nor a tool call -- genuinely stuck budget_exhausted hit max_iterations; says nothing about being on track simulated_user_failed OUR simulated-user model failed, not the agent chat_error the chat call raised mid-conversation (kda partial path) not_run the loop never started (conversation $ref skip) The default is BUDGET_EXHAUSTED and every other exit assigns explicitly, so a loop that simply runs out of range() is labelled correctly with no trailing else. Two exits were previously invisible and are the reason this is worth doing: - metric_skill catches SimulatedResponseError and breaks. A harness-side outage was scored against the product as metric_created=False/maql_correct=False. - kda_skill breaks on a chat error with a partial result. Deliberately NOT included: - No verdict changes. An exhausted run still fails. The point is that the cases become countable, not that any of them start passing. - No change to any max_iterations default (4-7, already tuned per kind). Whether a budget is too tight becomes answerable from data instead of argued -- which is the actual ask behind GDAI-2200, where ~13% of alert runs are estimated to need 7 turns against a ceiling of 6. Raising the ceiling first would have hidden the interaction with GDAI-2199's MANDATORY STOPs, which make prescribed end-turn-without-a-tool-call behaviour consume budget. - No try/except added to alert_skill's simulated-user call: there a failure already propagates as a hard error rather than being swallowed, which is the behaviour we want. Only metric_skill needed the label. Tests: existing detail assertions extended across all five kinds, plus dedicated coverage for budget_exhausted vs agent_silent vs success (including the turn the tool landed on), simulated_user_failed, and a regression guard asserting that two runs with identical scored booleans differ only in exit_reason -- the exact ambiguity this removes. 739 passed; ruff check clean. --- .../gooddata_eval/core/agentic/alert_skill.py | 26 ++++ .../core/agentic/conversation.py | 17 +++ .../gooddata_eval/core/agentic/kda_skill.py | 20 +++ .../core/agentic/metric_skill.py | 21 ++++ .../core/agentic/visualization.py | 18 +++ .../src/gooddata_eval/core/models.py | 41 ++++++ .../tests/test_agentic_alert_skill.py | 117 ++++++++++++++++++ .../tests/test_agentic_conversation.py | 4 + .../tests/test_agentic_kda_skill.py | 8 ++ .../tests/test_agentic_metric_skill.py | 67 ++++++++++ .../tests/test_agentic_visualization.py | 8 ++ 11 files changed, 347 insertions(+) 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 cb1addf3d..fe48c7309 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,7 @@ from gooddata_eval.core.models import ( AgenticAssertionError, AgenticEvalOutcome, + LoopExit, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown, @@ -473,6 +474,12 @@ class AlertRunResult: response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) reasoning_step_events: list[ReasoningStepEvent] = field(default_factory=list) + # Why the simulated-user loop stopped, and how many turns it took. Without these a run + # that ran out of turns is indistinguishable from one that refused: both land on + # alert_created=False, and every downstream check is `alert_created and ...`, so both + # also report operator/threshold/metric/recipients as False. + exit_reason: LoopExit = LoopExit.BUDGET_EXHAUSTED + turns_used: int = 0 @dataclass @@ -669,7 +676,12 @@ def _run_once(conv_id: str) -> AlertRunResult: conversation_history: list = [] current_question = question + # Defaults to BUDGET_EXHAUSTED: every other exit sets it explicitly, so a loop + # that simply runs out of range() is correctly labelled without a trailing else. + exit_reason = LoopExit.BUDGET_EXHAUSTED + turns_used = 0 for _iteration in range(max_iterations): + turns_used = _iteration + 1 chat_result = client.send_message(conv_id, current_question) reasoning_steps.extend(chat_result.reasoning_steps or []) response_id = chat_result.response_id or response_id @@ -684,6 +696,7 @@ def _run_once(conv_id: str) -> AlertRunResult: alert_id, actual_args, tool_called = _extract_alert_call(chat_result.tool_call_events or []) if tool_called: alert_id_to_delete = alert_id + exit_reason = LoopExit.SUCCESS break response_text = (chat_result.text_response or "").strip() if not response_text and chat_result.alert_proposals: @@ -692,10 +705,16 @@ def _run_once(conv_id: str) -> AlertRunResult: response_text = render_answer_text(chat_result) # Stop if agent gave a completely empty response (stuck) if not response_text and not chat_result.tool_call_events: + exit_reason = LoopExit.AGENT_SILENT break # Stop before generating a follow-up for the last iteration if _iteration >= max_iterations - 1: break + # No try/except here on purpose: a simulated-user failure in this evaluator + # already propagates as a hard error rather than being swallowed into a + # content failure, which is the behaviour we want. Contrast metric_skill, + # which catches SimulatedResponseError and breaks -- that one needs + # LoopExit.SIMULATED_USER_FAILED to stay distinguishable. follow_up = generate_simulated_alert_response( response_text, expected, conversation_history, question=question ) @@ -724,6 +743,8 @@ def _run_once(conv_id: str) -> AlertRunResult: response_id=response_id, tool_call_events=all_tool_call_events, reasoning_step_events=all_reasoning_step_events, + exit_reason=exit_reason, + turns_used=turns_used, ) finally: if alert_id_to_delete: @@ -886,6 +907,11 @@ def _write_scores(ctx: RunTraceContext) -> None: "attributes_correct": ev.attributes_correct, "granularity_correct": ev.granularity_correct, "actual_alert_arguments": best.actual_alert_arguments, + # Why the loop stopped. alert_created=False alone cannot tell a refusal from a run + # that hit max_iterations while still on track -- see LoopExit. + "exit_reason": best.exit_reason.value, + "turns_used": best.turns_used, + "max_iterations": max_iterations, "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index 919e4022c..d8534be02 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -29,6 +29,7 @@ AgenticAssertionError, AgenticEvalOutcome, ChatResult, + LoopExit, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown, @@ -93,6 +94,10 @@ class TurnResult(BaseModel): active_skills: list[str] = Field(default_factory=list) clarification_turns_used: int = 0 output_correct: bool | None = None + # Why this turn's clarification loop stopped -- see LoopExit. output_present=False alone + # cannot separate a turn that ran out of clarification budget from one where the agent + # went silent, and skill_success folds both into the same failure. + exit_reason: LoopExit = LoopExit.BUDGET_EXHAUSTED @property def skill_success(self) -> bool: @@ -112,6 +117,8 @@ def skill_success(self) -> bool: # What skill_routing was judged against -- without it, a turn showing # skill_routing=True and activated_skills=[] looks like a scoring bug. "active_skills", + # Why the clarification loop ended on this turn. + "exit_reason", } def detail(self) -> dict: @@ -421,6 +428,10 @@ def run_agentic_conversation( # read as "nothing was active", which is a different claim. active_skills=sorted(active_skills), output_correct=False, + # This turn's loop never ran at all -- a $ref pointing at an earlier + # turn's output could not be resolved. Labelling it BUDGET_EXHAUSTED + # (the field default) would claim it ran out of clarification turns. + exit_reason=LoopExit.NOT_RUN, ) ) continue @@ -431,6 +442,9 @@ def run_agentic_conversation( current_message = turn.message final_result: ChatResult | None = None + # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop + # that simply runs out of range() is labelled correctly with no trailing else. + turn_exit = LoopExit.BUDGET_EXHAUSTED for _iter in range(max_clarification_turns + 1): chat_result = client.send_message(conversation_id, current_message) final_result = chat_result @@ -447,6 +461,7 @@ def run_agentic_conversation( response_id = chat_result.response_id or response_id if _check_output_present(resolved_turn, chat_result): + turn_exit = LoopExit.SUCCESS break response_text = (chat_result.text_response or "").strip() @@ -455,6 +470,7 @@ def run_agentic_conversation( if not response_text: response_text = render_answer_text(chat_result) if not response_text and not chat_result.tool_call_events: + turn_exit = LoopExit.AGENT_SILENT break if clarification_turns >= max_clarification_turns: break @@ -500,6 +516,7 @@ def run_agentic_conversation( active_skills=sorted(active_skills), clarification_turns_used=clarification_turns, output_correct=output_correct, + exit_reason=turn_exit, ) ) 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 9eda50d68..0a261e6de 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 @@ -23,6 +23,7 @@ AgenticAssertionError, AgenticEvalOutcome, ChatResult, + LoopExit, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown, @@ -181,6 +182,10 @@ class KdaRunResult: response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) reasoning_step_events: list[ReasoningStepEvent] = field(default_factory=list) + # Why the simulated-user loop stopped -- see LoopExit. `triggered=False` alone cannot + # separate a refusal from a run that hit max_iterations while still on track. + exit_reason: LoopExit = LoopExit.BUDGET_EXHAUSTED + turns_used: int = 0 @dataclass @@ -268,7 +273,12 @@ def _accumulate(result: ChatResult) -> None: all_tool_call_events.extend(result.tool_call_events or []) all_reasoning_step_events.extend(result.reasoning_step_events or []) + # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that + # simply runs out of range() is labelled correctly with no trailing else. + exit_reason = LoopExit.BUDGET_EXHAUSTED + turns_used = 0 for iteration in range(max_iterations): + turns_used = iteration + 1 try: chat_result = client.send_message(conv_id, current_question) except Exception as exc: # noqa: BLE001 -- end this run, not the whole assertion @@ -282,6 +292,7 @@ def _accumulate(result: ChatResult) -> None: if create_args is not None: turn_wall_clock_sec = partial.turn_wall_clock_sec turn_completed = False + exit_reason = LoopExit.CHAT_ERROR break reasoning_steps.extend(chat_result.reasoning_steps or []) response_id = chat_result.response_id or response_id @@ -296,8 +307,10 @@ def _accumulate(result: ChatResult) -> None: # final either way -- execute_result may still be None (e.g. the skill's # 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 + exit_reason = LoopExit.SUCCESS break if not response_text: + exit_reason = LoopExit.AGENT_SILENT break if iteration >= max_iterations - 1: break @@ -313,6 +326,7 @@ def _accumulate(result: ChatResult) -> None: 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) + exit_reason = LoopExit.SIMULATED_USER_FAILED break ev = _evaluate_run(create_args, execute_result, turn_completed, disambiguated) @@ -326,6 +340,8 @@ def _accumulate(result: ChatResult) -> None: response_id=response_id, tool_call_events=all_tool_call_events, reasoning_step_events=all_reasoning_step_events, + exit_reason=exit_reason, + turns_used=turns_used, ) try: @@ -482,6 +498,10 @@ def _write_scores(ctx: RunTraceContext) -> None: "disambiguated": ev.disambiguated, "actual_create_args": best.actual_create_args, "actual_execute_result": best.actual_execute_result, + # Why the loop stopped -- see LoopExit. + "exit_reason": best.exit_reason.value, + "turns_used": best.turns_used, + "max_iterations": max_iterations, "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py index 7d8f18454..1b9fc9a70 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py @@ -27,6 +27,7 @@ from gooddata_eval.core.models import ( AgenticAssertionError, AgenticEvalOutcome, + LoopExit, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown, @@ -159,6 +160,11 @@ class MetricRunResult: response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) reasoning_step_events: list[ReasoningStepEvent] = field(default_factory=list) + # Why the simulated-user loop stopped. metric_created=False alone cannot separate a run + # that ran out of turns, one where the agent went silent, and one where the harness's + # own simulated user failed -- see LoopExit. + exit_reason: LoopExit = LoopExit.BUDGET_EXHAUSTED + turns_used: int = 0 timings: PhaseTimings = field(default_factory=PhaseTimings) @@ -256,6 +262,9 @@ def _execute_single_metric_run( reasoning_index_offset = 0 try: + # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that + # simply runs out of range() is labelled correctly with no trailing else. + exit_reason = LoopExit.BUDGET_EXHAUSTED for _iteration in range(max_iterations): turns += 1 agent_started = time.monotonic() @@ -282,11 +291,13 @@ def _execute_single_metric_run( f"{agent_elapsed:.2f}s; metric result received" ) metric_result = candidate + exit_reason = LoopExit.SUCCESS break response_text = (chat_result.text_response or "").strip() if not response_text: response_text = render_answer_text(chat_result) if not response_text and not chat_result.tool_call_events: + exit_reason = LoopExit.AGENT_SILENT break if _iteration >= max_iterations - 1: break @@ -305,6 +316,9 @@ def _execute_single_metric_run( f"[timer] metric_skill {conversation_id} gpt-4o-mini simulated user failed after " f"{simulated_elapsed:.2f}s" ) + # A harness-side fault, not an agent one. Recorded so reporting can treat it + # as an error instead of scoring the agent down for it. + exit_reason = LoopExit.SIMULATED_USER_FAILED break simulated_elapsed = time.monotonic() - simulated_started timings.simulated_user_s += simulated_elapsed @@ -328,6 +342,8 @@ def _execute_single_metric_run( tool_call_events=all_tool_call_events, reasoning_step_events=all_reasoning_step_events, timings=timings, + exit_reason=exit_reason, + turns_used=turns, ) finally: for metric_id in created_metric_ids: @@ -492,6 +508,11 @@ def _write_scores(ctx: RunTraceContext) -> None: "maql_correct": best.maql_correct, "expected_maql_candidates": [c.get("maql", "") for c in expected_outputs_list], "actual_maql": best.actual_maql, + # Why the loop stopped. metric_created=False alone cannot tell a refusal from a run + # that hit max_iterations while still on track -- see LoopExit. + "exit_reason": best.exit_reason.value, + "turns_used": best.turns_used, + "max_iterations": max_iterations, "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py index 12faaaffb..16e34a559 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py @@ -32,6 +32,7 @@ AgenticAssertionError, AgenticEvalOutcome, CreatedVisualization, + LoopExit, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown, @@ -57,6 +58,9 @@ class RunResult: response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) reasoning_step_events: list[ReasoningStepEvent] = field(default_factory=list) + # Why the simulated-user loop stopped. visualization_created=False alone cannot separate + # a refusal from a run that hit max_iterations while still on track -- see LoopExit. + exit_reason: LoopExit = LoopExit.BUDGET_EXHAUSTED @dataclass @@ -191,6 +195,9 @@ def _execute_single_run( current_result = client.send_message(conversation_id, question) + # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that + # simply runs out of range() is labelled correctly with no trailing else. + exit_reason = LoopExit.BUDGET_EXHAUSTED for iteration in range(max_iterations): total_turns += 1.0 total_steps += float(current_result.reasoning_step_count) @@ -207,9 +214,14 @@ def _execute_single_run( viz_produced = bool(current_result.created_visualizations and current_result.created_visualizations.objects) if viz_produced: + exit_reason = LoopExit.SUCCESS break + # render_answer_text (QA-29230) is the broader emptiness test: a turn carrying + # search results but no plain text is not silent. Keeping it means AGENT_SILENT + # labels only genuinely empty turns. response_text = render_answer_text(current_result) if not response_text: + exit_reason = LoopExit.AGENT_SILENT break if iteration >= max_iterations - 1: break @@ -235,6 +247,7 @@ def _execute_single_run( response_id=response_id, tool_call_events=all_tool_call_events, reasoning_step_events=all_reasoning_step_events, + exit_reason=exit_reason, ) @@ -422,6 +435,11 @@ def _write_scores(ctx: RunTraceContext) -> None: ev = best.eval_result detail = { **evaluation_result_detail(ev), + # Why the loop stopped -- see LoopExit. total_turns is already the turn count for + # this run, so it doubles as turns_used. + "exit_reason": best.exit_reason.value, + "turns_used": int(best.total_turns), + "max_iterations": max_iterations, "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/models.py b/packages/gooddata-eval/src/gooddata_eval/core/models.py index a1f1d5165..7abeda0d6 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/models.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/models.py @@ -6,6 +6,7 @@ import json import re +from enum import Enum from typing import Any from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -13,6 +14,46 @@ from gooddata_eval.core.timing import PhaseTimings +class LoopExit(str, Enum): + """Why an agentic evaluator's simulated-user loop stopped. + + Every agentic kind drives the agent through a loop of simulated-user turns that can end + several ways, but the result only ever recorded *whether* the agent produced its output + -- so a run that ran out of turns while doing the right thing was reported identically + to one that refused, and identically to one that answered wrongly. Downstream checks are + all of the form ``produced_output and ``, so an exhausted run also reports every + per-field check as False: specific-sounding content failures for work the agent was + never given the chance to do. + + This does not change any verdict -- an exhausted run still fails. It makes the three + cases countable, so "is the turn budget too tight" becomes a question the data can + answer rather than one that has to be argued. + """ + + SUCCESS = "success" + """The agent produced the expected output; the loop broke early.""" + + AGENT_SILENT = "agent_silent" + """The agent returned neither text nor a tool call -- genuinely stuck.""" + + BUDGET_EXHAUSTED = "budget_exhausted" + """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.""" + + 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.""" + + CHAT_ERROR = "chat_error" + """The chat call itself raised mid-conversation. Like SIMULATED_USER_FAILED this is + infrastructure rather than agent capability, but it comes from the GoodData side.""" + + NOT_RUN = "not_run" + """The loop never started -- e.g. an agentic_conversation turn whose $ref to an earlier + turn's output could not be resolved, so the turn was skipped before any message was + sent. Distinct from every other member, which all imply at least one turn happened.""" + + class AacQueryField(BaseModel): model_config = ConfigDict(extra="allow") diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index cf812609d..168e06977 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -690,6 +690,9 @@ def test_evaluate_agentic_alert_skill_returns_reasoning_steps_on_pass(): "attributes_correct": True, "granularity_correct": True, "actual_alert_arguments": {"operator": "GREATER_THAN", "threshold": 500}, + "exit_reason": "success", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } @@ -730,6 +733,11 @@ def test_evaluate_agentic_alert_skill_attaches_reasoning_steps_to_exception_on_f "attributes_correct": False, "granularity_correct": False, "actual_alert_arguments": {}, + # The agent replied but never called create_metric_alert, and the loop had only one + # iteration to give -- budget_exhausted, not a refusal. + "exit_reason": "budget_exhausted", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } @@ -996,3 +1004,112 @@ def test_every_gen_ai_interval_is_accepted(): assert AnomalyDetectionGranularity.parse(value.lower()) is AnomalyDetectionGranularity(value) assert AnomalyDetectionGranularity.parse(None) is None assert AnomalyDetectionGranularity.parse(" ") is None + + +# --- LoopExit: why the simulated-user loop stopped ------------------------------------------ +# alert_created=False is reached three different ways, and before exit_reason existed they +# were indistinguishable in the result. Every downstream check is `alert_created and ...`, so +# a run that merely ran out of turns also reported operator/threshold/metric/recipients as +# False -- six specific-sounding content failures for work never attempted. + + +def _text_only_result(text="Which dashboard should I bind it to?"): + return ChatResult.model_validate({"text_response": text, "toolCallEvents": [], "reasoningSteps": []}) + + +def _run_alert(mock_client, *, max_iterations, simulated_reply="the revenue one"): + with _patched(mock_client, simulated_reply=simulated_reply, delete_alert=True): + try: + outcome = evaluate_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Notify me whenever the number of orders goes above 500", + expected_output={"operator": "GREATER_THAN", "threshold": 500}, + k=1, + max_iterations=max_iterations, + ) + return outcome.detail + except AlertSkillAssertionError as exc: + return exc.detail + + +def test_exit_reason_budget_exhausted_when_the_agent_keeps_talking(): + """The agent is responsive and on-topic but never reaches the tool inside the budget.""" + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _text_only_result() + + detail = _run_alert(mock_client, max_iterations=3) + + assert detail["exit_reason"] == "budget_exhausted" + assert detail["turns_used"] == 3 + assert detail["max_iterations"] == 3 + assert detail["alert_created"] is False + + +def test_exit_reason_agent_silent_is_not_budget_exhausted(): + """An empty response stops the loop early -- a different failure from running out of turns.""" + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = ChatResult.model_validate( + {"text_response": "", "toolCallEvents": [], "reasoningSteps": []} + ) + + detail = _run_alert(mock_client, max_iterations=6) + + assert detail["exit_reason"] == "agent_silent" + # Stopped on the first turn rather than burning all six -- the distinction the field exists for. + assert detail["turns_used"] == 1 + assert detail["alert_created"] is False + + +def test_exit_reason_success_records_the_turn_the_tool_landed_on(): + """Turn count is the point: it says how much of the budget a passing run needed.""" + created = ChatResult.model_validate( + { + "text_response": "Alert created.", + "toolCallEvents": [ + { + "functionName": "create_metric_alert", + "functionArguments": '{"operator": "GREATER_THAN", "threshold": 500}', + "result": '{"id": "alert-1"}', + } + ], + "reasoningSteps": [], + } + ) + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [_text_only_result(), _text_only_result(), created] + + detail = _run_alert(mock_client, max_iterations=6) + + assert detail["exit_reason"] == "success" + assert detail["turns_used"] == 3 + assert detail["alert_created"] is True + + +def test_budget_exhausted_and_agent_silent_are_otherwise_identical(): + """The regression guard: without exit_reason these two are the same result. + + Both fail, both report every per-field check False. If a future change drops the field + or stops assigning it, this is what catches it. + """ + talky = MagicMock() + talky.create_conversation.return_value = "conv-1" + talky.send_message.return_value = _text_only_result() + + silent = MagicMock() + silent.create_conversation.return_value = "conv-1" + silent.send_message.return_value = ChatResult.model_validate( + {"text_response": "", "toolCallEvents": [], "reasoningSteps": []} + ) + + scored = ("alert_created", "operator_correct", "threshold_correct", "metric_correct", "recipients_correct") + d_talky = _run_alert(talky, max_iterations=2) + d_silent = _run_alert(silent, max_iterations=2) + + assert {k: d_talky[k] for k in scored} == {k: d_silent[k] for k in scored} + assert all(d_talky[k] is False for k in scored) + assert d_talky["exit_reason"] != d_silent["exit_reason"] diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index dd39f9996..8adb83b4a 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -957,6 +957,7 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): "output_correct": None, "activated_skills": ["visualization"], "active_skills": ["visualization"], + "exit_reason": "success", } ], "latency_breakdown": [], @@ -1021,6 +1022,9 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ "output_correct": None, "activated_skills": ["other_skill"], "active_skills": ["other_skill"], + # Output never appeared and the clarification budget ran out -- the turn is + # not a refusal, which skill_routing/output_present alone cannot show. + "exit_reason": "budget_exhausted", } ], "latency_breakdown": [], diff --git a/packages/gooddata-eval/tests/test_agentic_kda_skill.py b/packages/gooddata-eval/tests/test_agentic_kda_skill.py index f6eecf8f9..cce3f4dda 100644 --- a/packages/gooddata-eval/tests/test_agentic_kda_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_kda_skill.py @@ -1132,6 +1132,9 @@ def test_evaluate_agentic_kda_skill_returns_reasoning_steps_on_pass(): "disambiguated": False, "actual_create_args": {"measure": {"type": "metric", "id": "revenue"}}, "actual_execute_result": {"success": True, "data": {"summary": {}}}, + "exit_reason": "success", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } @@ -1165,6 +1168,11 @@ def test_evaluate_agentic_kda_skill_attaches_reasoning_steps_to_exception_on_fai "disambiguated": False, "actual_create_args": None, "actual_execute_result": None, + # The agent answered but never called create -- the loop simply ran out of turns. + # triggered/executed/success are all False, none of which says that. + "exit_reason": "budget_exhausted", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index bc36fe4f0..74d2b70b6 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -660,6 +660,9 @@ def test_evaluate_agentic_metric_skill_returns_reasoning_steps_on_pass(): "maql_correct": True, "expected_maql_candidates": ["SELECT {metric/foo}"], "actual_maql": "SELECT {metric/foo}", + "exit_reason": "success", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } @@ -689,6 +692,9 @@ def test_evaluate_agentic_metric_skill_attaches_reasoning_steps_to_exception_on_ "maql_correct": False, "expected_maql_candidates": ["SELECT {metric/foo}"], "actual_maql": "", + "exit_reason": "budget_exhausted", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } assert exc_info.value.conversation_id == "conv-1" @@ -764,3 +770,64 @@ def test_no_timer_output_by_default(monkeypatch, capsys): assert "[timer]" not in capsys.readouterr().out # Silenced, not un-measured. assert summary.run_results[0].timings.agent_s == 3.0 + + +# --- LoopExit: a harness fault must not read as an agent failure ----------------------------- + + +def test_exit_reason_simulated_user_failed_is_not_an_agent_failure(): + """metric_skill catches SimulatedResponseError and breaks -- silently, before this field. + + The harness's own simulated user failing produced exactly the same result as the agent + getting the MAQL wrong: metric_created=False, maql_correct=False. That is a harness + outage scored against the product, and nothing in the output said so. + """ + mock_client = _client() + mock_client.send_message.return_value = ChatResult.model_validate( + {"textResponse": "Which measure did you mean?", "toolCallEvents": [], "reasoningSteps": []} + ) + + with ( + _patched(mock_client, simulated_error=SimulatedResponseError("simulated user unavailable")), + pytest.raises(MetricSkillAssertionError) as exc_info, + ): + evaluate_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=6, + ) + + detail = exc_info.value.detail + assert detail["exit_reason"] == "simulated_user_failed" + # Broke on turn 1 of a 6-turn budget: not the agent running out of room, and not a refusal. + assert detail["turns_used"] == 1 + assert detail["metric_created"] is False + + +def test_exit_reason_budget_exhausted_differs_from_simulated_user_failure(): + """Same scored booleans, different cause -- the distinction the field exists to make.""" + mock_client = _client() + mock_client.send_message.return_value = ChatResult.model_validate( + {"textResponse": "Which measure did you mean?", "toolCallEvents": [], "reasoningSteps": []} + ) + + with _patched(mock_client, simulated_reply="the revenue one"), pytest.raises(MetricSkillAssertionError) as exc: + evaluate_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=3, + ) + + detail = exc.value.detail + assert detail["exit_reason"] == "budget_exhausted" + assert detail["turns_used"] == 3 + assert detail["max_iterations"] == 3 + assert detail["metric_created"] is False diff --git a/packages/gooddata-eval/tests/test_agentic_visualization.py b/packages/gooddata-eval/tests/test_agentic_visualization.py index 766313d74..7a77a1b2c 100644 --- a/packages/gooddata-eval/tests/test_agentic_visualization.py +++ b/packages/gooddata-eval/tests/test_agentic_visualization.py @@ -320,6 +320,9 @@ def test_evaluate_agentic_visualization_returns_reasoning_steps_on_pass(): "actual_dim_uris": ["label/date.quarter"], "expected_filters": {"date": [], "ranking": [], "attribute": []}, "actual_filters": {"date": [], "ranking": [], "attribute": []}, + "exit_reason": "success", + "turns_used": 1, + "max_iterations": 4, "latency_breakdown": [], } @@ -371,5 +374,10 @@ def test_evaluate_agentic_visualization_attaches_reasoning_steps_to_exception_on "actual_dim_uris": [], "expected_filters": {"date": [], "ranking": [], "attribute": []}, "actual_filters": {"date": [], "ranking": [], "attribute": []}, + # No visualization and only one iteration available: the loop ran out of budget. + # Every check above reads False, which is exactly why exit_reason has to be here. + "exit_reason": "budget_exhausted", + "turns_used": 1, + "max_iterations": 1, "latency_breakdown": [], } From 5c819b1777f833bb8c3d4d77406f3a349d5aa75e Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 9 Sep 2026 16:52:48 +0200 Subject: [PATCH 2/3] fix(gooddata-eval): address CodeRabbit review on the loop exit_reason work All three findings were valid. 1. visualization reported turns_used=0 after sending a request. The initial send_message happens before the loop, but total_turns only incremented inside it -- so max_iterations=0 sent one message and claimed zero turns. Counted at the send instead, with the loop's first pass skipping its own increment to compensate; verified turns_used == send_message call count for every (max_iterations, break point) combination, not just the edge case. 2. agentic_conversation was the exception to the new detail contract: it had no turns_used equivalent and no budget. It already tracked clarification_turns_used per turn and simply never reported it -- now in _DETAIL_FIELDS -- and ConversationResult carries max_clarification_turns so detail can state the limit the way every other kind does. LoopExit.NOT_RUN already reports 0, since that path never touches the counter. 3. The two early-termination tests asserted exit_reason and turns_used but not max_iterations, so a wrong or missing limit would have passed unnoticed. Adds a parametrized regression test over max_iterations 0..3 asserting total_turns equals the number of requests actually sent, which is the invariant finding 1 broke. 785 passed; ruff check clean; ruff format delta unchanged from master's baseline. --- .../core/agentic/conversation.py | 10 ++++++- .../core/agentic/visualization.py | 8 +++++- .../tests/test_agentic_alert_skill.py | 1 + .../tests/test_agentic_conversation.py | 4 +++ .../tests/test_agentic_metric_skill.py | 1 + .../tests/test_agentic_visualization.py | 26 +++++++++++++++++++ 6 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index d8534be02..99f58011c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -117,8 +117,10 @@ def skill_success(self) -> bool: # What skill_routing was judged against -- without it, a turn showing # skill_routing=True and activated_skills=[] looks like a scoring bug. "active_skills", - # Why the clarification loop ended on this turn. + # Why the clarification loop ended on this turn, and how much of the budget it + # took to get there -- exit_reason alone cannot be related to the limit without it. "exit_reason", + "clarification_turns_used", } def detail(self) -> dict: @@ -343,6 +345,10 @@ class ConversationResult: full_skill_coverage: bool conversation_success: bool total_clarification_turns: int + # The configured per-turn clarification budget, so a reader can tell a turn that used + # its whole allowance from one that stopped early. Every other agentic kind reports its + # limit in detail; without this, conversation is the exception to that contract. + max_clarification_turns: int = _DEFAULT_MAX_CLARIFICATION_TURNS reasoning_steps: list[str] = field(default_factory=list) response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) @@ -539,6 +545,7 @@ def run_agentic_conversation( full_skill_coverage=full_skill_coverage, conversation_success=conversation_success, total_clarification_turns=total_clarification_turns, + max_clarification_turns=max_clarification_turns, reasoning_steps=reasoning_steps, response_id=response_id, tool_call_events=conversation_tool_call_events, @@ -550,6 +557,7 @@ def _conversation_detail(result: ConversationResult) -> dict: return { "full_skill_coverage": result.full_skill_coverage, "total_clarification_turns": result.total_clarification_turns, + "max_clarification_turns": result.max_clarification_turns, "turns": [tr.detail() for tr in result.turn_results], "latency_breakdown": build_latency_breakdown(result.tool_call_events, result.reasoning_step_events), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py index 16e34a559..5f48ac3c3 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py @@ -194,12 +194,18 @@ def _execute_single_run( simulated_response_guide = expected_outputs[0] # primary candidate guides the simulated user current_result = client.send_message(conversation_id, question) + # Counted here, not at the top of the loop: this request is sent unconditionally, so + # with max_iterations=0 the loop body never runs and total_turns would report 0 turns + # for a conversation the agent did receive. The loop's own increment is skipped on its + # first pass to compensate, keeping total_turns == number of send_message calls. + total_turns += 1.0 # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that # simply runs out of range() is labelled correctly with no trailing else. exit_reason = LoopExit.BUDGET_EXHAUSTED for iteration in range(max_iterations): - total_turns += 1.0 + if iteration: + total_turns += 1.0 total_steps += float(current_result.reasoning_step_count) turn_offset, tool_index_offset, reasoning_index_offset = shift_and_index_events( current_result, diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index 168e06977..07b6f6ec0 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -1061,6 +1061,7 @@ def test_exit_reason_agent_silent_is_not_budget_exhausted(): assert detail["exit_reason"] == "agent_silent" # Stopped on the first turn rather than burning all six -- the distinction the field exists for. assert detail["turns_used"] == 1 + assert detail["max_iterations"] == 6 assert detail["alert_created"] is False diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 8adb83b4a..8245ba4ba 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -948,6 +948,7 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): assert outcome.detail == { "full_skill_coverage": True, "total_clarification_turns": 0, + "max_clarification_turns": 7, "turns": [ { "turn_id": "t1", @@ -958,6 +959,7 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): "activated_skills": ["visualization"], "active_skills": ["visualization"], "exit_reason": "success", + "clarification_turns_used": 0, } ], "latency_breakdown": [], @@ -1013,6 +1015,7 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ assert exc_info.value.detail == { "full_skill_coverage": False, "total_clarification_turns": 0, + "max_clarification_turns": 0, "turns": [ { "turn_id": "t1", @@ -1025,6 +1028,7 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ # Output never appeared and the clarification budget ran out -- the turn is # not a refusal, which skill_routing/output_present alone cannot show. "exit_reason": "budget_exhausted", + "clarification_turns_used": 0, } ], "latency_breakdown": [], diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 74d2b70b6..37d155eb2 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -805,6 +805,7 @@ def test_exit_reason_simulated_user_failed_is_not_an_agent_failure(): assert detail["exit_reason"] == "simulated_user_failed" # Broke on turn 1 of a 6-turn budget: not the agent running out of room, and not a refusal. assert detail["turns_used"] == 1 + assert detail["max_iterations"] == 6 assert detail["metric_created"] is False diff --git a/packages/gooddata-eval/tests/test_agentic_visualization.py b/packages/gooddata-eval/tests/test_agentic_visualization.py index 7a77a1b2c..fe336fa88 100644 --- a/packages/gooddata-eval/tests/test_agentic_visualization.py +++ b/packages/gooddata-eval/tests/test_agentic_visualization.py @@ -381,3 +381,29 @@ def test_evaluate_agentic_visualization_attaches_reasoning_steps_to_exception_on "max_iterations": 1, "latency_breakdown": [], } + + +# --- turns_used counts every send_message, including the pre-loop one ------------------------ + + +@pytest.mark.parametrize("max_iterations", [0, 1, 2, 3]) +def test_turns_used_equals_the_number_of_requests_sent(max_iterations): + """The initial request is sent before the loop, so counting only loop passes undercounts. + + With max_iterations=0 the loop body never runs, yet the agent has already received one + message -- reporting turns_used=0 there would claim a conversation that did happen never + did. The agent never produces a visualization here, so no run breaks early and the count + is driven purely by the budget. + """ + client = MagicMock() + client.send_message.return_value = ChatResult.model_validate( + {"textResponse": "Which metric did you mean?", "createdVisualizations": None} + ) + expected = [CreatedVisualization.model_validate({"id": "v", "type": "COLUMN", "query": {"fields": {}}})] + + with patch("gooddata_eval.core.agentic.visualization.generate_simulated_response", return_value="the revenue one"): + run = _execute_single_run(client, "conv-1", "chart revenue", expected, max_iterations=max_iterations) + + assert int(run.total_turns) == client.send_message.call_count + # A request was sent regardless of the budget, so the count is never zero. + assert int(run.total_turns) >= 1 From d0eb73a428bd0f218168b3da00a63dcbb965dd0b Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 9 Sep 2026 22:51:56 +0200 Subject: [PATCH 3/3] fix(gooddata-eval): record chat and simulated-user faults instead of aborting the item LoopExit declared CHAT_ERROR and SIMULATED_USER_FAILED, but only kda_skill could produce either: every other kind let the exception escape its runner. Since the K-run loop appends results as it goes, an exception on run 2 discarded run 1 along with any exit_reason -- an infrastructure blip erased the results of the runs that had worked, and the item reported no loop-exit at all. The contract this PR introduces was therefore unreachable in four of the five kinds. Both faults are now caught where kda_skill already catches them: - alert_skill, metric_skill, conversation, visualization record CHAT_ERROR and end the run, harvesting exc.partial_result where the surrounding code already knows how to consume one. - alert_skill and visualization record SIMULATED_USER_FAILED. alert_skill deliberately let this propagate, to keep a harness fault from being scored as a content failure; the exit reason achieves that same separation -- reporting reads it to classify the run as an error -- while keeping the completed runs. - visualization guards its opening request too, where there is no partial conversation to evaluate, and skips the loop rather than scoring a stale result. - conversation's no_error was hardcoded True on the reasoning that a chat fault would have escaped before reaching it. It now reads the exit reason back. The catch is ChatError, not Exception: a bug in our own code must still surface as a crash rather than be relabelled as a GoodData-side fault. kda_skill catches Exception broadly and is left alone here, but the two should agree. Co-Authored-By: Claude Opus 5 --- .../gooddata_eval/core/agentic/alert_skill.py | 32 +++++-- .../core/agentic/conversation.py | 25 ++++- .../core/agentic/metric_skill.py | 13 ++- .../core/agentic/visualization.py | 41 ++++++-- .../tests/test_agentic_alert_skill.py | 93 ++++++++++++++++++- .../tests/test_agentic_conversation.py | 62 ++++++++++++- .../tests/test_agentic_metric_skill.py | 29 +++++- .../tests/test_agentic_visualization.py | 57 +++++++++++- 8 files changed, 325 insertions(+), 27 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 fe48c7309..f27205873 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 @@ -22,7 +22,7 @@ utc_now, ) from gooddata_eval.core.chat.render import render_answer_text -from gooddata_eval.core.chat.sse_client import ChatClient +from gooddata_eval.core.chat.sse_client import ChatClient, ChatError from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.models import ( AgenticAssertionError, @@ -682,7 +682,14 @@ def _run_once(conv_id: str) -> AlertRunResult: turns_used = 0 for _iteration in range(max_iterations): turns_used = _iteration + 1 - chat_result = client.send_message(conv_id, current_question) + try: + chat_result = client.send_message(conv_id, current_question) + except ChatError as exc: + # Without this the exception escapes run_agentic_alert_skill entirely, + # discarding every K-run already completed along with any exit_reason. + print(f"[CHAT] send_message failed for conversation {conv_id}: {exc}") + exit_reason = LoopExit.CHAT_ERROR + break reasoning_steps.extend(chat_result.reasoning_steps or []) response_id = chat_result.response_id or response_id turn_offset, tool_index_offset, reasoning_index_offset = shift_and_index_events( @@ -710,14 +717,19 @@ def _run_once(conv_id: str) -> AlertRunResult: # Stop before generating a follow-up for the last iteration if _iteration >= max_iterations - 1: break - # No try/except here on purpose: a simulated-user failure in this evaluator - # already propagates as a hard error rather than being swallowed into a - # content failure, which is the behaviour we want. Contrast metric_skill, - # which catches SimulatedResponseError and breaks -- that one needs - # LoopExit.SIMULATED_USER_FAILED to stay distinguishable. - follow_up = generate_simulated_alert_response( - response_text, expected, conversation_history, question=question - ) + # Recorded rather than raised, matching metric_skill and kda_skill. Letting it + # propagate did keep a harness fault from being scored as a content failure, + # but it also discarded the K-runs already completed -- and SIMULATED_USER_FAILED + # achieves the same separation while keeping them, since reporting reads the + # 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 + ) + 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}") + exit_reason = LoopExit.SIMULATED_USER_FAILED + break # 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/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index 99f58011c..9b098a6cc 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -23,7 +23,7 @@ from gooddata_eval.core.agentic.alert_skill import render_alert_proposal from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids, _extract_metric_result from gooddata_eval.core.chat.render import render_answer_text -from gooddata_eval.core.chat.sse_client import ChatClient +from gooddata_eval.core.chat.sse_client import ChatClient, ChatError from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.models import ( AgenticAssertionError, @@ -452,7 +452,23 @@ def run_agentic_conversation( # that simply runs out of range() is labelled correctly with no trailing else. turn_exit = LoopExit.BUDGET_EXHAUSTED for _iter in range(max_clarification_turns + 1): - chat_result = client.send_message(conversation_id, current_message) + try: + chat_result = client.send_message(conversation_id, current_message) + except ChatError as exc: + # Recorded rather than raised so the turns already completed keep their + # results, and so this turn is distinguishable from one where the agent + # simply failed to produce output. no_error below reads this back. + print(f"[CHAT] send_message failed for conversation {conversation_id}: {exc}") + partial = getattr(exc, "partial_result", None) + if partial is not None: + final_result = partial + all_tool_calls.extend(partial.tool_call_events or []) + conversation_tool_call_events.extend(partial.tool_call_events or []) + conversation_reasoning_step_events.extend(partial.reasoning_step_events or []) + reasoning_steps.extend(partial.reasoning_steps or []) + response_id = partial.response_id or response_id + turn_exit = LoopExit.CHAT_ERROR + break final_result = chat_result turn_offset, tool_index_offset, reasoning_index_offset = shift_and_index_events( chat_result, @@ -517,7 +533,10 @@ def run_agentic_conversation( expected_skill=turn.expected_skill, skill_routing=skill_routing, output_present=output_present, - no_error=True, # SDK raises on errors; reaching here means no critical error. + # A chat fault used to escape the whole run, so reaching here did mean no + # error. Now that it is caught and recorded, this has to read it back -- + # otherwise a turn whose chat call failed reports no_error=True. + no_error=turn_exit is not LoopExit.CHAT_ERROR, activated_skills=declared or [], active_skills=sorted(active_skills), clarification_turns_used=clarification_turns, diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py index 1b9fc9a70..dc5700192 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py @@ -21,7 +21,7 @@ utc_now, ) from gooddata_eval.core.chat.render import render_answer_text -from gooddata_eval.core.chat.sse_client import ChatClient +from gooddata_eval.core.chat.sse_client import ChatClient, ChatError from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.evaluators._maql import normalize_maql from gooddata_eval.core.models import ( @@ -268,7 +268,16 @@ def _execute_single_metric_run( for _iteration in range(max_iterations): turns += 1 agent_started = time.monotonic() - chat_result = client.send_message(conversation_id, current_question) + try: + chat_result = client.send_message(conversation_id, current_question) + except ChatError as exc: + # Without this the exception escapes run_agentic_metric_skill entirely, + # discarding every K-run already completed along with any exit_reason. A + # GoodData-side fault, so recorded like the simulated-user one below. + timings.agent_s += time.monotonic() - agent_started + print(f"[CHAT] send_message failed for conversation {conversation_id}: {exc}") + exit_reason = LoopExit.CHAT_ERROR + break agent_elapsed = time.monotonic() - agent_started timings.agent_s += agent_elapsed reasoning_steps.extend(chat_result.reasoning_steps or []) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py index 5f48ac3c3..fefe1c941 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py @@ -20,7 +20,7 @@ utc_now, ) from gooddata_eval.core.chat.render import render_answer_text -from gooddata_eval.core.chat.sse_client import ChatClient +from gooddata_eval.core.chat.sse_client import ChatClient, ChatError from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.evaluators.visualization import ( EvaluationResult, @@ -31,6 +31,7 @@ from gooddata_eval.core.models import ( AgenticAssertionError, AgenticEvalOutcome, + ChatResult, CreatedVisualization, LoopExit, ReasoningStepEvent, @@ -193,17 +194,26 @@ def _execute_single_run( tool_index_offset = 0 # ditto for ToolCallEvent.index simulated_response_guide = expected_outputs[0] # primary candidate guides the simulated user - current_result = client.send_message(conversation_id, question) + # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that + # simply runs out of range() is labelled correctly with no trailing else. + exit_reason = LoopExit.BUDGET_EXHAUSTED + + try: + current_result = client.send_message(conversation_id, question) + except ChatError as exc: + # The opening request, so there is no partial conversation to evaluate: fall through + # to the empty-result path with CHAT_ERROR recorded. Raising here would discard every + # K-run already completed along with any exit_reason. + print(f"[CHAT] send_message failed for conversation {conversation_id}: {exc}") + current_result = getattr(exc, "partial_result", None) or ChatResult() + exit_reason = LoopExit.CHAT_ERROR # Counted here, not at the top of the loop: this request is sent unconditionally, so # with max_iterations=0 the loop body never runs and total_turns would report 0 turns # for a conversation the agent did receive. The loop's own increment is skipped on its # first pass to compensate, keeping total_turns == number of send_message calls. total_turns += 1.0 - # Defaults to BUDGET_EXHAUSTED: every other exit assigns explicitly, so a loop that - # simply runs out of range() is labelled correctly with no trailing else. - exit_reason = LoopExit.BUDGET_EXHAUSTED - for iteration in range(max_iterations): + for iteration in range(max_iterations if exit_reason is not LoopExit.CHAT_ERROR else 0): if iteration: total_turns += 1.0 total_steps += float(current_result.reasoning_step_count) @@ -232,8 +242,23 @@ def _execute_single_run( if iteration >= max_iterations - 1: break - follow_up = generate_simulated_response(response_text, simulated_response_guide) - current_result = client.send_message(conversation_id, follow_up) + try: + follow_up = generate_simulated_response(response_text, simulated_response_guide) + except Exception as exc: # noqa: BLE001 -- harness-side fault; end only this run + print(f"[SIM-USER] Simulated reply failed for conversation {conversation_id}: {exc}") + exit_reason = LoopExit.SIMULATED_USER_FAILED + break + try: + current_result = client.send_message(conversation_id, follow_up) + except ChatError as exc: + # Unlike the opening request there IS a conversation to evaluate here, so the + # last good result stands and whatever the agent had produced is still scored. + print(f"[CHAT] send_message failed for conversation {conversation_id}: {exc}") + partial = getattr(exc, "partial_result", None) + if partial is not None: + current_result = partial + exit_reason = LoopExit.CHAT_ERROR + break skill_activated = _check_visualization_skill_activated(all_tool_call_events) actual_output: CreatedVisualization | None = None diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index 07b6f6ec0..a6776cf79 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -21,7 +21,8 @@ render_alert_proposal, run_agentic_alert_skill, ) -from gooddata_eval.core.models import ChatResult +from gooddata_eval.core.chat.sse_client import ChatError +from gooddata_eval.core.models import ChatResult, LoopExit _DATE_FILTER = { "relativeDateFilter": { @@ -1017,6 +1018,23 @@ def _text_only_result(text="Which dashboard should I bind it to?"): return ChatResult.model_validate({"text_response": text, "toolCallEvents": [], "reasoningSteps": []}) +def _alert_created_result() -> ChatResult: + """A turn where the agent actually calls the tool -- the run passes.""" + return ChatResult.model_validate( + { + "text_response": "Alert created.", + "toolCallEvents": [ + { + "functionName": "create_metric_alert", + "functionArguments": '{"operator": "GREATER_THAN", "threshold": 500}', + "result": '{"id": "alert-1"}', + } + ], + "reasoningSteps": [], + } + ) + + def _run_alert(mock_client, *, max_iterations, simulated_reply="the revenue one"): with _patched(mock_client, simulated_reply=simulated_reply, delete_alert=True): try: @@ -1114,3 +1132,76 @@ def test_budget_exhausted_and_agent_silent_are_otherwise_identical(): assert {k: d_talky[k] for k in scored} == {k: d_silent[k] for k in scored} assert all(d_talky[k] is False for k in scored) assert d_talky["exit_reason"] != d_silent["exit_reason"] + + +def test_exit_reason_chat_error_keeps_the_run_instead_of_losing_the_whole_item(): + """A mid-run chat fault used to escape run_agentic_alert_skill entirely. + + That discarded every K-run already completed along with any exit_reason, so an + infrastructure blip on the last run erased the results of the ones that had worked. + It is now recorded on the run, leaving the fault distinguishable from an agent that + simply never created the alert. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = ChatError("gen-ai fell over") + + detail = _run_alert(mock_client, max_iterations=3) + + assert detail["exit_reason"] == "chat_error" + assert detail["alert_created"] is False + # Distinct from the budget case: the loop stopped on turn 1, it did not use its 3. + assert detail["turns_used"] == 1 + + +def test_exit_reason_simulated_user_failed_is_recorded_not_raised(): + """The harness's own simulated user failing is a harness fault, not an agent one. + + It used to propagate as a hard error, which did keep it from being scored as a content + failure -- but also threw away the completed K-runs. SIMULATED_USER_FAILED separates the + two just as well while keeping them, matching metric_skill and kda_skill. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _text_only_result() + + with _patched(mock_client, simulated_reply="unused", delete_alert=True) as mock_sim: + mock_sim.side_effect = RuntimeError("openai down") + try: + detail = evaluate_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Notify me whenever the number of orders goes above 500", + expected_output={"operator": "GREATER_THAN", "threshold": 500}, + k=1, + max_iterations=3, + ).detail + except AlertSkillAssertionError as exc: + detail = exc.detail + + assert detail["exit_reason"] == "simulated_user_failed" + assert detail["alert_created"] is False + + +def test_a_chat_error_on_a_later_run_does_not_discard_the_earlier_ones(): + """K-run preservation, which is the whole point of catching rather than raising.""" + mock_client = MagicMock() + mock_client.create_conversation.side_effect = ["conv-1", "conv-2"] + mock_client.send_message.side_effect = [_alert_created_result(), ChatError("boom")] + + with _patched(mock_client, delete_alert=True): + summary = run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Notify me whenever the number of orders goes above 500", + expected_output={"operator": "GREATER_THAN", "threshold": 500}, + k=2, + max_iterations=3, + ) + + assert len(summary.run_results) == 2 + assert summary.run_results[0].exit_reason is LoopExit.SUCCESS + assert summary.run_results[1].exit_reason is LoopExit.CHAT_ERROR + assert summary.pass_at_k is True # run 0 still counts diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 8245ba4ba..cfad39fe8 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -13,7 +13,8 @@ evaluate_agentic_conversation, run_agentic_conversation, ) -from gooddata_eval.core.models import ChatResult, ToolCallEvent +from gooddata_eval.core.chat.sse_client import ChatError +from gooddata_eval.core.models import ChatResult, LoopExit, ToolCallEvent def _skills_tc(*skills): @@ -1033,3 +1034,62 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ ], "latency_breakdown": [], } + + +def test_a_chat_error_ends_only_its_own_turn_and_is_recorded(): + """A chat fault used to escape the whole conversation, discarding the turns already done. + + t1 completes; t2's chat call fails. t1's result must survive, and t2 must be reported as + an infrastructure fault -- both exit_reason and no_error say so, so it is not counted as + the agent failing to produce output. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("m1")]), + ChatError("stream died"), + ] + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + ): + result = run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=_two_metric_turn_fixture(), + ) + + assert len(result.turn_results) == 2 + assert result.turn_results[0].exit_reason is LoopExit.SUCCESS + assert result.turn_results[0].output_present is True + + assert result.turn_results[1].exit_reason is LoopExit.CHAT_ERROR + assert result.turn_results[1].output_present is False + # no_error used to be hardcoded True on the reasoning that a chat fault would have + # escaped before reaching here. Now that it is caught, it has to read the exit back. + assert result.turn_results[1].no_error is False + assert result.turn_results[1].skill_success is False + + +def test_a_non_chat_exception_still_propagates(): + """Only chat faults are absorbed. A programming error must not be relabelled CHAT_ERROR.""" + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("m1")]), + TypeError("a real bug"), + ] + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + pytest.raises(TypeError), + ): + run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=_two_metric_turn_fixture(), + ) diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 37d155eb2..db2c14928 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -18,7 +18,8 @@ generate_simulated_response, run_agentic_metric_skill, ) -from gooddata_eval.core.models import ChatResult, ToolCallEvent +from gooddata_eval.core.chat.sse_client import ChatError +from gooddata_eval.core.models import ChatResult, LoopExit, ToolCallEvent from gooddata_eval.core.timing import TIMERS_ENV_VAR # --- time.monotonic() side effects --------------------------------------------------- @@ -832,3 +833,29 @@ def test_exit_reason_budget_exhausted_differs_from_simulated_user_failure(): assert detail["turns_used"] == 3 assert detail["max_iterations"] == 3 assert detail["metric_created"] is False + + +def test_chat_error_ends_the_run_and_is_recorded_rather_than_raised(): + """A mid-run chat fault used to escape run_agentic_metric_skill entirely. + + That discarded every K-run already completed. It is now recorded like the + simulated-user fault beside it, so an infrastructure blip stays distinguishable from + the agent failing to create the metric. + """ + mock_client = _client() + mock_client.send_message.side_effect = ChatError("gen-ai fell over") + + with _patched(mock_client): + summary = run_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=3, + ) + + assert summary.run_results[0].exit_reason is LoopExit.CHAT_ERROR + assert summary.run_results[0].metric_created is False + assert summary.pass_at_k is False diff --git a/packages/gooddata-eval/tests/test_agentic_visualization.py b/packages/gooddata-eval/tests/test_agentic_visualization.py index fe336fa88..b25dbbc98 100644 --- a/packages/gooddata-eval/tests/test_agentic_visualization.py +++ b/packages/gooddata-eval/tests/test_agentic_visualization.py @@ -13,7 +13,8 @@ evaluate_agentic_visualization, run_agentic_visualization, ) -from gooddata_eval.core.models import ChatResult, CreatedVisualization +from gooddata_eval.core.chat.sse_client import ChatError +from gooddata_eval.core.models import ChatResult, CreatedVisualization, LoopExit def _viz(id_: str = "v1") -> dict: @@ -407,3 +408,57 @@ def test_turns_used_equals_the_number_of_requests_sent(max_iterations): assert int(run.total_turns) == client.send_message.call_count # A request was sent regardless of the budget, so the count is never zero. assert int(run.total_turns) >= 1 + + +# ── chat faults end the run, they do not abort the item ───────────────────── + + +def test_execute_single_run_records_chat_error_on_the_opening_request(): + """The very first send_message failing leaves no conversation to evaluate. + + It must still return a RunResult rather than raise: raising discards the K-runs already + completed. The run scores as no-visualization, but exit_reason says infrastructure, not + a refusal. + """ + client = MagicMock() + client.send_message.side_effect = ChatError("gen-ai unavailable") + + result = _execute_single_run(client, "conv-1", "Show revenue", [_expected()]) + + assert result.exit_reason is LoopExit.CHAT_ERROR + assert result.actual_output is None + assert result.eval_result.visualization_created is False + # The request was still sent, so it counts -- the same rule the max_iterations=0 case follows. + assert result.total_turns == 1.0 + # The loop must not run after the opening failure. + assert client.send_message.call_count == 1 + + +def test_execute_single_run_records_chat_error_on_a_follow_up_request(monkeypatch): + """A later failure keeps whatever the conversation had already produced.""" + monkeypatch.setattr( + "gooddata_eval.core.agentic.visualization.generate_simulated_response", lambda *a, **k: "the revenue one" + ) + client = MagicMock() + client.send_message.side_effect = [_chat_clarification(), ChatError("stream died")] + + result = _execute_single_run(client, "conv-1", "Show revenue", [_expected()]) + + assert result.exit_reason is LoopExit.CHAT_ERROR + assert client.send_message.call_count == 2 + + +def test_execute_single_run_records_simulated_user_failure_separately(monkeypatch): + """A harness-side fault must not read as the agent failing to produce a chart.""" + + def _boom(*_a, **_k): + raise RuntimeError("openai down") + + monkeypatch.setattr("gooddata_eval.core.agentic.visualization.generate_simulated_response", _boom) + client = MagicMock() + client.send_message.return_value = _chat_clarification() + + result = _execute_single_run(client, "conv-1", "Show revenue", [_expected()]) + + assert result.exit_reason is LoopExit.SIMULATED_USER_FAILED + assert result.eval_result.visualization_created is False