Merge pull request #1753 from gooddata/QA-29053-fix-metric-turn-extraction

fix(gooddata-eval): keep retrying create_metric result, not the first failed one

Author: FrankHuynh

packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py

@@ -12,7 +12,7 @@
 from pydantic import BaseModel
 
 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
+from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids, _extract_metric_result
 from gooddata_eval.core.chat.sse_client import ChatClient
 from gooddata_eval.core.config import ReasoningEffort
 from gooddata_eval.core.models import AgenticEvalOutcome, ChatResult, ToolCallEvent
@@ -120,7 +120,7 @@ def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool
             and getattr(chat_result.created_visualizations, "objects", chat_result.created_visualizations)
         )
     if otype == "metric":
-        return any(tc.function_name == "create_metric" for tc in (chat_result.tool_call_events or []))
+        return _extract_metric_result(chat_result.tool_call_events or []) is not None
     if otype == "tool_call":
         expected_tool = turn.expected_tool_name
         if not expected_tool:
@@ -129,19 +129,6 @@ def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool
     return False
 
 
-def _extract_metric_from_turn(tool_call_events: list[ToolCallEvent]) -> dict | None:
-    """Extract the result payload from the create_metric tool call, if present."""
-    for tc in tool_call_events:
-        if tc.function_name != "create_metric":
-            continue
-        if not tc.result:
-            continue
-        result_data = tc.parsed_result()
-        if result_data is not None:
-            return result_data.get("data", result_data)
-    return None
-
-
 def _check_output_correct(turn: TurnDefinition, chat_result: ChatResult) -> bool | None:
     """Check output correctness against expected_output when defined.
 
@@ -186,7 +173,7 @@ def _check_output_correct(turn: TurnDefinition, chat_result: ChatResult) -> bool
         return all(results) if results else None
 
     if otype == "metric":
-        metric_result = _extract_metric_from_turn(chat_result.tool_call_events or [])
+        metric_result = _extract_metric_result(chat_result.tool_call_events or [])
         if not metric_result:
             return False
         return _normalize_maql(metric_result.get("maql", "")) == _normalize_maql(expected.get("maql", ""))
@@ -362,7 +349,7 @@ def run_agentic_conversation(
 
             # Capture metric output for $ref resolution in subsequent turns.
             if final_result and turn.expected_output_type == "metric":
-                metric_data = _extract_metric_from_turn(all_tool_calls)
+                metric_data = _extract_metric_result(all_tool_calls)
                 if metric_data:
                     turn_outputs[turn.turn_id] = metric_data
 

packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py

@@ -163,11 +163,22 @@ class AgenticMetricSummary:
 
 
 def _extract_metric_result(tool_call_events: list[ToolCallEvent]) -> dict | None:
-    for tc in tool_call_events:
-        if tc.function_name == "create_metric" and tc.result:
-            result_data = tc.parsed_result()
-            if result_data is not None:
-                return result_data.get("data", result_data)
+    """Result payload of the create_metric tool call.
+
+    Prefers the most recent successful call in this turn -- when the agent retries
+    after a validation error, the earlier failed attempt must not shadow it. Shared
+    with ``conversation.py``, which imports this instead of keeping its own copy.
+    """
+    for tc in reversed(tool_call_events):
+        if tc.function_name != "create_metric" or not tc.result:
+            continue
+        result_data = tc.parsed_result()
+        if not isinstance(result_data, dict):
+            continue
+        payload = result_data.get("data", result_data)
+        if not isinstance(payload, dict) or not payload or payload.get("isError"):
+            continue
+        return payload
     return None
 
 
@@ -223,7 +234,7 @@ def _execute_single_metric_run(
     """
     primary_expected = expected_outputs[0] if expected_outputs else {}
     metric_result: dict | None = None
-    metric_id_to_delete: str | None = None
+    created_metric_ids: list[str] = []
     turns = 0
     current_question = question
     reasoning_steps: list[str] = []
@@ -235,10 +246,12 @@ def _execute_single_metric_run(
             chat_result = client.send_message(conversation_id, current_question)
             reasoning_steps.extend(chat_result.reasoning_steps or [])
             response_id = chat_result.response_id or response_id
+            for metric_id in _extract_created_metric_ids(chat_result.tool_call_events or []):
+                if metric_id not in created_metric_ids:
+                    created_metric_ids.append(metric_id)
             candidate = _extract_metric_result(chat_result.tool_call_events or [])
             if candidate is not None:
                 metric_result = candidate
-                metric_id_to_delete = candidate.get("metric_id")
                 break
             response_text = (chat_result.text_response or "").strip()
             if not response_text and not chat_result.tool_call_events:
@@ -265,8 +278,8 @@ def _execute_single_metric_run(
             response_id=response_id,
         )
     finally:
-        if metric_id_to_delete:
-            _delete_metric(sdk, workspace_id, metric_id_to_delete)
+        for metric_id in created_metric_ids:
+            _delete_metric(sdk, workspace_id, metric_id)
 
 
 def run_agentic_metric_skill(

packages/gooddata-eval/tests/test_agentic_conversation.py

@@ -30,6 +30,14 @@ def _create_metric_tc(metric_id):
     return tc
 
 
+def _create_metric_tc_error(message):
+    tc = MagicMock(spec=ToolCallEvent)
+    tc.function_name = "create_metric"
+    tc.result = "{}"  # truthy; content comes from parsed_result
+    tc.parsed_result = lambda msg=message: {"data": {"isError": True, "error": {"text": msg}}}
+    return tc
+
+
 def _metric_turn_result(tool_calls):
     r = MagicMock()
     r.text_response = "done"
@@ -471,6 +479,47 @@ def test_run_agentic_conversation_records_a_failed_turn_when_a_ref_cannot_be_res
     assert result.conversation_success is False
 
 
+def test_run_agentic_conversation_sends_the_next_turn_after_a_self_corrected_retry():
+    """QA-29053 regression: turn 1 self-corrects create_metric after a failed first attempt;
+    turn 2's message must still be sent, resolving its $ref against the successful retry."""
+    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_error("invalid MAQL"), _create_metric_tc("m1")]),
+        _viz_turn_result(text="Here is your chart", viz=[MagicMock()], tool_calls=[_skills_tc("visualization")]),
+    ]
+    fixture = ConversationFixture(
+        id="test-retry",
+        expected_skills=["metric", "visualization"],
+        turns=[
+            TurnDefinition(turn_id="t1", message="Create it", expected_skill="metric", expected_output_type="metric"),
+            TurnDefinition(
+                turn_id="t2",
+                message="Chart it",
+                expected_skill="visualization",
+                expected_output={"metrics": ["metric/$ref:t1.metric_id"]},
+            ),
+        ],
+    )
+
+    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=fixture,
+        )
+
+    assert mock_client.send_message.call_count == 2
+    mock_client.send_message.assert_any_call("conv-1", "Chart it")
+    assert result.turn_results[0].skill_success is True
+    assert result.turn_results[1].no_error is True
+    assert result.conversation_success is True
+
+
 def test_run_agentic_conversation_accumulates_reasoning_steps_across_turns():
     mock_client = MagicMock()
     mock_client.create_conversation.return_value = "conv-1"

packages/gooddata-eval/tests/test_agentic_metric_skill.py

@@ -12,12 +12,75 @@
     MetricSkillAssertionError,
     SimulatedResponseError,
     _delete_metric,
+    _extract_metric_result,
     _normalize_maql,
     evaluate_agentic_metric_skill,
     generate_simulated_response,
     run_agentic_metric_skill,
 )
-from gooddata_eval.core.models import ChatResult
+from gooddata_eval.core.models import ChatResult, ToolCallEvent
+
+
+def _create_metric_call(result: str) -> ToolCallEvent:
+    return ToolCallEvent(function_name="create_metric", function_arguments="{}", result=result)
+
+
+_FAILED_RESULT = '{"data": {"isError": true, "error": {"text": "invalid MAQL"}}}'
+
+
+def test_extract_metric_result_skips_a_failed_retry_and_returns_the_successful_one():
+    """QA-29053 regression: agent self-corrects an invalid MAQL by retrying create_metric
+    within the same turn; the successful retry must be captured, not the failed first call."""
+    calls = [
+        _create_metric_call(_FAILED_RESULT),
+        _create_metric_call('{"data": {"metric_id": "m1", "maql": "SELECT {metric/foo}"}}'),
+    ]
+    assert _extract_metric_result(calls) == {"metric_id": "m1", "maql": "SELECT {metric/foo}"}
+
+
+def test_extract_metric_result_returns_none_when_every_attempt_failed():
+    calls = [_create_metric_call(_FAILED_RESULT), _create_metric_call(_FAILED_RESULT)]
+    assert _extract_metric_result(calls) is None
+
+
+def test_extract_metric_result_skips_a_failed_call_after_an_earlier_success():
+    # The failed call is last, so reversed() reaches it first and must skip past it.
+    calls = [_create_metric_call('{"data": {"metric_id": "m1"}}'), _create_metric_call(_FAILED_RESULT)]
+    assert _extract_metric_result(calls) == {"metric_id": "m1"}
+
+
+def test_extract_metric_result_prefers_the_most_recent_successful_call():
+    """Two distinct successful create_metric calls in one turn (not a retry after a
+    failure) -- the later one wins."""
+    calls = [
+        _create_metric_call('{"data": {"metric_id": "m1"}}'),
+        _create_metric_call('{"data": {"metric_id": "m2"}}'),
+    ]
+    assert _extract_metric_result(calls) == {"metric_id": "m2"}
+
+
+def test_extract_metric_result_skips_a_non_dict_payload():
+    # The non-dict payload is last, so reversed() reaches it first and must skip past it.
+    calls = [
+        _create_metric_call('{"data": {"metric_id": "m2"}}'),
+        _create_metric_call('{"data": [{"metric_id": "m1"}]}'),
+    ]
+    assert _extract_metric_result(calls) == {"metric_id": "m2"}
+
+
+def test_extract_metric_result_skips_a_non_dict_decoded_result():
+    # The whole decoded result (not just its "data" field) is a non-dict here.
+    calls = [_create_metric_call('{"metric_id": "m2"}'), _create_metric_call("[]")]
+    assert _extract_metric_result(calls) == {"metric_id": "m2"}
+
+
+def test_extract_metric_result_skips_an_empty_payload():
+    # The empty payload is last, so reversed() reaches it first and must skip past it.
+    calls = [
+        _create_metric_call('{"data": {"metric_id": "m2"}}'),
+        _create_metric_call('{"data": {}}'),
+    ]
+    assert _extract_metric_result(calls) == {"metric_id": "m2"}
 
 
 def test_normalize_maql_strips_whitespace():
@@ -269,6 +332,49 @@ def test_run_agentic_metric_skill_deletes_created_metric():
     mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric")
 
 
+def test_run_agentic_metric_skill_deletes_the_metric_created_by_a_self_corrected_retry():
+    """QA-29053 regression: a failed create_metric call followed by a successful retry, in the
+    same turn, used to leave metric_id_to_delete unset -- the metric the retry created leaked
+    into the shared workspace."""
+    mock_client = MagicMock()
+    mock_client.create_conversation.return_value = "conv-1"
+    mock_client.send_message.return_value = ChatResult.model_validate(
+        {
+            "textResponse": "done",
+            "toolCallEvents": [
+                {
+                    "functionName": "create_metric",
+                    "functionArguments": "{}",
+                    "result": '{"data": {"isError": true, "error": {"text": "invalid MAQL"}}}',
+                },
+                {
+                    "functionName": "create_metric",
+                    "functionArguments": "{}",
+                    "result": '{"data": {"maql": "SELECT {metric/foo}", "metric_id": "foo_metric"}}',
+                },
+            ],
+            "reasoningStepCount": 1,
+        }
+    )
+    with (
+        patch("gooddata_eval.core.agentic.metric_skill.ChatClient", return_value=mock_client),
+        patch("gooddata_eval.core.agentic.metric_skill.GoodDataSdk") as mock_sdk_cls,
+    ):
+        mock_sdk = mock_sdk_cls.create.return_value
+        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=1,
+        )
+    assert summary.best.metric_created is True
+    assert summary.best.maql_correct is True
+    mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric")
+
+
 def test_run_agentic_metric_skill_deletes_metric_even_when_teardown_fails():
     # A metric is created, then conversation teardown raises; the created metric must still
     # have been cleaned up (its deletion happens inside the per-run finally, before teardown).