fix(gooddata-eval): read set_skills' result rather than its arguments to determine active skills

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

@@ -295,22 +295,49 @@ def _replace(match: re.Match) -> str:  # type: ignore[type-arg]
     return json.loads(resolved_raw)
 
 
+def _extract_active_skills(tc: ToolCallEvent) -> list[str]:
+    """Derive the active skills from a set_skills ToolCallEvent.
+
+    Prefers the authoritative post-replacement active skills echoed back in the tool
+    result (e.g. `skills_to_activate`), falling back to requested arguments (`skill_names`)
+    when the result is absent, unparseable, or errored.
+    """
+    if getattr(tc, "result", None) or hasattr(tc, "parsed_result"):
+        try:
+            result_data = tc.parsed_result()
+        except Exception:
+            result_data = None
+        if isinstance(result_data, dict):
+            payload = result_data.get("data", result_data)
+            if isinstance(payload, dict) and payload.get("status") not in ("error", "failure"):
+                for key in ("skills_to_activate", "skill_names", "skills"):
+                    val = payload.get(key)
+                    if isinstance(val, list):
+                        return [str(s) for s in val]
+        elif isinstance(result_data, list):
+            return [str(s) for s in result_data]
+
+    args = tc.parsed_arguments() if hasattr(tc, "parsed_arguments") else {}
+    args = args or {}
+    names = args.get("skill_names")
+    if names is None:
+        names = args.get("skills")
+    if isinstance(names, list):
+        return [str(s) for s in names]
+    return list(names or [])
+
+
 def _set_skills_declarations(tool_call_events: list[ToolCallEvent]) -> list[list[str]]:
     """Every set_skills declaration in these events, in call order.
 
-    `skill_names` is the key the tool declares; `skills` is a legacy spelling kept as a
-    fallback. A call carrying neither is treated as declaring an empty list, which is what
-    the platform would do with one.
+    Prefers the authoritative post-replacement set echoed back in each call's result,
+    falling back to requested skill arguments when the result is absent or unparseable.
     """
     declarations: list[list[str]] = []
     for tc in tool_call_events:
         if tc.function_name != "set_skills":
             continue
-        args = tc.parsed_arguments() or {}
-        names = args.get("skill_names")
-        if names is None:
-            names = args.get("skills")
-        declarations.append(list(names or []))
+        declarations.append(_extract_active_skills(tc))
     return declarations
 
 

packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py

@@ -211,13 +211,44 @@ def requires_execution_of(expected_output: object) -> bool:
     return isinstance(expected_output, dict) and expected_output.get("requires_execution") is True
 
 
+def _extract_active_skills(tc: ToolCallEvent) -> list[str]:
+    """Derive the active skills from a set_skills ToolCallEvent.
+
+    Prefers the authoritative post-replacement active skills echoed back in the tool
+    result (e.g. `skills_to_activate`), falling back to requested arguments (`skill_names`)
+    when the result is absent, unparseable, or errored.
+    """
+    if getattr(tc, "result", None) or hasattr(tc, "parsed_result"):
+        try:
+            result_data = tc.parsed_result()
+        except Exception:
+            result_data = None
+        if isinstance(result_data, dict):
+            payload = result_data.get("data", result_data)
+            if isinstance(payload, dict) and payload.get("status") not in ("error", "failure"):
+                for key in ("skills_to_activate", "skill_names", "skills"):
+                    val = payload.get(key)
+                    if isinstance(val, list):
+                        return [str(s) for s in val]
+        elif isinstance(result_data, list):
+            return [str(s) for s in result_data]
+
+    args = tc.parsed_arguments() if hasattr(tc, "parsed_arguments") else {}
+    args = args or {}
+    names = args.get("skill_names")
+    if names is None:
+        names = args.get("skills")
+    if isinstance(names, list):
+        return [str(s) for s in names]
+    return list(names or [])
+
+
 def _check_visualization_skill_activated(tool_call_events: list[ToolCallEvent]) -> bool:
-    """Return True if set_skills was called with 'visualization' in skill_names."""
+    """Return True if set_skills activated 'visualization' (reading result, fallback to args)."""
     for tc in tool_call_events:
         if tc.function_name == "set_skills":
-            args = tc.parsed_arguments()
-            skill_names = args.get("skill_names", [])
-            if isinstance(skill_names, list) and "visualization" in skill_names:
+            active = _extract_active_skills(tc)
+            if "visualization" in active:
                 return True
     return False
 

packages/gooddata-eval/tests/test_agentic_conversation.py

@@ -30,9 +30,11 @@
     _check_output_correct,
     _created_alert_ids,
     _expected_viz,
+    _final_skill_declaration,
     _get_sim_user_response,
     _metric_creations,
     _resolve_refs,
+    _set_skills_declarations,
     evaluate_agentic_conversation,
     resolve_conversation_mode,
     run_agentic_conversation,
@@ -44,7 +46,7 @@
 from gooddata_eval.core.models import ChatResult, LoopExit, ToolCallEvent
 
 
-def _skills_tc(*skills):
+def _skills_tc(*skills, result=None):
     tc = MagicMock(spec=ToolCallEvent)
     tc.call_ts = None
     tc.result_ts = None
@@ -54,6 +56,16 @@ def _skills_tc(*skills):
     # previously used a bare `skills`, which only passed via _activated_skills' fallback
     # spelling -- so they exercised a payload shape the platform never actually sends.
     tc.parsed_arguments = lambda: {"skill_names": list(skills)}
+    if result is not None:
+        tc.result = _json.dumps(result) if not isinstance(result, str) else result
+        tc.parsed_result = lambda: (
+            (_json.loads(result) if isinstance(result, str) and result.strip().startswith("{") else result)
+            if not isinstance(result, dict)
+            else result
+        )
+    else:
+        tc.result = None
+        tc.parsed_result = lambda: None
     return tc
 
 
@@ -631,6 +643,123 @@ def test_run_agentic_conversation_skill_routing_false_when_skill_never_activated
     assert result.turn_results[0].skill_routing is False
 
 
+def test_set_skills_declarations_prefers_result_over_arguments():
+    """Authoritative active skills echoed back in the result take precedence over arguments."""
+    tc = _skills_tc("metric", result={"skills_to_activate": ["visualization"]})
+    assert _set_skills_declarations([tc]) == [["visualization"]]
+    assert _final_skill_declaration([tc]) == ["visualization"]
+
+
+def test_set_skills_declarations_drops_unrecognised_requested_name():
+    """When the agent requests an unrecognised/retired skill that the platform drops,
+    it is not credited in declarations."""
+    tc = _skills_tc("retired_skill", "metric", result={"skills_to_activate": ["metric"]})
+    assert _set_skills_declarations([tc]) == [["metric"]]
+    assert _final_skill_declaration([tc]) == ["metric"]
+
+
+def test_set_skills_declarations_credits_implicit_dependency():
+    """When the platform pulls in a dependency not explicitly requested by the agent,
+    it is credited in declarations."""
+    tc = _skills_tc("dashboard_builder", result={"skills_to_activate": ["dashboard_builder", "visualization"]})
+    assert _set_skills_declarations([tc]) == [["dashboard_builder", "visualization"]]
+    assert _final_skill_declaration([tc]) == ["dashboard_builder", "visualization"]
+
+
+def test_set_skills_declarations_falls_back_when_result_missing():
+    """Older captured traces with result=None fall back to arguments."""
+    tc = _skills_tc("metric", result=None)
+    assert _set_skills_declarations([tc]) == [["metric"]]
+    assert _final_skill_declaration([tc]) == ["metric"]
+
+
+def test_set_skills_declarations_falls_back_when_result_unparseable():
+    """Unparseable non-JSON result falls back to arguments."""
+    tc = _skills_tc("metric", result="Internal Error 500")
+    assert _set_skills_declarations([tc]) == [["metric"]]
+    assert _final_skill_declaration([tc]) == ["metric"]
+
+
+def test_set_skills_declarations_falls_back_when_call_errored():
+    """An errored tool call falls back to arguments."""
+    tc = _skills_tc("metric", result={"status": "error", "error": "failed"})
+    assert _set_skills_declarations([tc]) == [["metric"]]
+    assert _final_skill_declaration([tc]) == ["metric"]
+
+
+def test_set_skills_declarations_empty_result_clears_skills():
+    """An explicit empty list in result is respected and does not fall back to arguments."""
+    tc = _skills_tc("metric", result={"skills_to_activate": []})
+    assert _set_skills_declarations([tc]) == [[]]
+    assert _final_skill_declaration([tc]) == []
+
+
+def test_run_agentic_conversation_unrecognised_skill_dropped_by_result_fails_routing():
+    """A fixture expecting a misspelled skill that the service drops fails routing."""
+    mock_client = MagicMock()
+    mock_client.create_conversation.return_value = "conv-1"
+    # Agent asked for "unknown_skill", but platform dropped it and returned "metric"
+    tc = _skills_tc("unknown_skill", result={"skills_to_activate": ["metric"]})
+    mock_client.send_message.return_value = _metric_turn_result([tc, _create_metric_tc("m1")])
+
+    fixture = ConversationFixture(
+        id="test-unrecognised-skill-dropped",
+        expected_skills=["unknown_skill"],
+        turns=[
+            TurnDefinition(
+                turn_id="t1", message="Do something", expected_skill="unknown_skill", expected_output_type="metric"
+            ),
+        ],
+    )
+    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 result.turn_results[0].skill_routing is False
+    assert result.turn_results[0].active_skills == ["metric"]
+    assert result.full_skill_coverage is False
+
+
+def test_run_agentic_conversation_dependency_pulled_in_by_result_passes_routing():
+    """A skill activated implicitly as a dependency is credited in routing and coverage."""
+    mock_client = MagicMock()
+    mock_client.create_conversation.return_value = "conv-1"
+    # Agent asked for "dashboard_builder", service activated dashboard_builder + metric as dependency
+    tc = _skills_tc("dashboard_builder", result={"skills_to_activate": ["dashboard_builder", "metric"]})
+    mock_client.send_message.return_value = _metric_turn_result([tc, _create_metric_tc("m1")])
+
+    fixture = ConversationFixture(
+        id="test-dependency-credited",
+        expected_skills=["metric"],
+        turns=[
+            TurnDefinition(
+                turn_id="t1", message="Do something", expected_skill="metric", expected_output_type="metric"
+            ),
+        ],
+    )
+    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 result.turn_results[0].skill_routing is True
+    assert "metric" in result.turn_results[0].active_skills
+    assert result.full_skill_coverage is True
+
+
 def test_run_agentic_conversation_deletes_metrics_even_when_a_later_turn_raises():
     mock_client = MagicMock()
     mock_client.create_conversation.return_value = "conv-1"

packages/gooddata-eval/tests/test_visualization_evaluator.py

@@ -1,4 +1,5 @@
 # (C) 2026 GoodData Corporation
+import json
 import re
 from datetime import date
 
@@ -106,6 +107,103 @@ def test_evaluator_skill_not_activated_when_wrong_skill_name():
     assert result.detail["skill_activated"] is False
 
 
+def test_evaluator_skill_unrecognised_name_in_arguments_dropped_by_result():
+    """When arguments requested 'visualization' but the service result dropped it,
+    it must not be credited as active."""
+    ev = get_evaluator("visualization")
+    chat = ChatResult.model_validate(
+        {
+            "createdVisualizations": {"objects": [_expected()], "reasoning": ""},
+            "toolCallEvents": [
+                {
+                    "functionName": "set_skills",
+                    "functionArguments": '{"skill_names": ["visualization"]}',
+                    "result": json.dumps({"skills_to_activate": ["search"]}),
+                }
+            ],
+        }
+    )
+    result = ev.evaluate(_item(_expected()), chat)
+    assert result.detail["skill_activated"] is False
+
+
+def test_evaluator_skill_dependency_pulled_in_by_result_is_credited():
+    """When arguments did not name 'visualization' but the service result pulled it in
+    as a dependency, it must be credited as active."""
+    ev = get_evaluator("visualization")
+    chat = ChatResult.model_validate(
+        {
+            "createdVisualizations": {"objects": [_expected()], "reasoning": ""},
+            "toolCallEvents": [
+                {
+                    "functionName": "set_skills",
+                    "functionArguments": '{"skill_names": ["dashboard_builder"]}',
+                    "result": json.dumps({"skills_to_activate": ["dashboard_builder", "visualization"]}),
+                }
+            ],
+        }
+    )
+    result = ev.evaluate(_item(_expected()), chat)
+    assert result.detail["skill_activated"] is True
+
+
+def test_evaluator_skill_fallback_when_result_missing():
+    """When the result is missing (legacy trace), fallback to arguments."""
+    ev = get_evaluator("visualization")
+    chat = ChatResult.model_validate(
+        {
+            "createdVisualizations": {"objects": [_expected()], "reasoning": ""},
+            "toolCallEvents": [
+                {
+                    "functionName": "set_skills",
+                    "functionArguments": '{"skill_names": ["visualization"]}',
+                    "result": None,
+                }
+            ],
+        }
+    )
+    result = ev.evaluate(_item(_expected()), chat)
+    assert result.detail["skill_activated"] is True
+
+
+def test_evaluator_skill_fallback_when_result_unparseable():
+    """When the result is unparseable non-JSON text, fallback to arguments."""
+    ev = get_evaluator("visualization")
+    chat = ChatResult.model_validate(
+        {
+            "createdVisualizations": {"objects": [_expected()], "reasoning": ""},
+            "toolCallEvents": [
+                {
+                    "functionName": "set_skills",
+                    "functionArguments": '{"skill_names": ["visualization"]}',
+                    "result": "502 Bad Gateway",
+                }
+            ],
+        }
+    )
+    result = ev.evaluate(_item(_expected()), chat)
+    assert result.detail["skill_activated"] is True
+
+
+def test_evaluator_skill_fallback_when_call_errored():
+    """When the result indicates the tool call errored, fallback to arguments."""
+    ev = get_evaluator("visualization")
+    chat = ChatResult.model_validate(
+        {
+            "createdVisualizations": {"objects": [_expected()], "reasoning": ""},
+            "toolCallEvents": [
+                {
+                    "functionName": "set_skills",
+                    "functionArguments": '{"skill_names": ["visualization"]}',
+                    "result": json.dumps({"status": "error", "message": "Failed to set skills"}),
+                }
+            ],
+        }
+    )
+    result = ev.evaluate(_item(_expected()), chat)
+    assert result.detail["skill_activated"] is True
+
+
 def _ranked(attribute: str | None, dim_alias: str = "d_q"):
     """Single-dimension chart with a top-1 ranking filter, optionally naming the attribute."""
     rank = {"type": "ranking_filter", "using": "m_rev", "top": 1}