From 23532485ff1d40c4295c214184ab580a7fd599d1 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Mon, 24 Aug 2026 22:04:38 +0200 Subject: [PATCH 1/4] Use a TurnDetail dataclass instead of a dict literal in _conversation_detail Addresses hkad98's PR #1750 review comment: the per-turn subset reported in detail["turns"] was a bare dict literal with no type checking. TurnDetail mirrors the same 6 fields; asdict() at the boundary keeps the output shape (and the detail: dict contract) unchanged. --- .../core/agentic/conversation.py | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 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 a87338df3..4c6c03b43 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -5,7 +5,7 @@ import json import re -from dataclasses import dataclass, field +from dataclasses import asdict, dataclass, field from typing import Literal from gooddata_sdk import GoodDataSdk @@ -426,19 +426,33 @@ def run_agentic_conversation( ) +@dataclass +class TurnDetail: + """The subset of a TurnResult reported in detail["turns"] for one conversation turn.""" + + turn_id: str + expected_skill: str + skill_routing: bool + output_present: bool + output_correct: bool | None + activated_skills: list[str] + + def _conversation_detail(result: ConversationResult) -> dict: return { "full_skill_coverage": result.full_skill_coverage, "total_clarification_turns": result.total_clarification_turns, "turns": [ - { - "turn_id": tr.turn_id, - "expected_skill": tr.expected_skill, - "skill_routing": tr.skill_routing, - "output_present": tr.output_present, - "output_correct": tr.output_correct, - "activated_skills": tr.activated_skills, - } + asdict( + TurnDetail( + turn_id=tr.turn_id, + expected_skill=tr.expected_skill, + skill_routing=tr.skill_routing, + output_present=tr.output_present, + output_correct=tr.output_correct, + activated_skills=tr.activated_skills, + ) + ) for tr in result.turn_results ], "latency_breakdown": build_latency_breakdown(result.tool_call_events, result.reasoning_step_events), From 25494ea7be179264ad494ea90750af3500f2bc57 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 25 Aug 2026 10:49:02 +0200 Subject: [PATCH 2/4] Move turn detail dict-building onto TurnResult.detail() Per hkad98's review comment on #1757: replace the separate TurnDetail dataclass + _conversation_detail's asdict() call with a detail() method on TurnResult itself, since it already owns every field being reported. Output shape (detail["turns"]) is unchanged. --- .../core/agentic/conversation.py | 39 +++++++------------ 1 file changed, 13 insertions(+), 26 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 4c6c03b43..f409593a9 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -5,7 +5,7 @@ import json import re -from dataclasses import asdict, dataclass, field +from dataclasses import dataclass, field from typing import Literal from gooddata_sdk import GoodDataSdk @@ -70,6 +70,17 @@ class TurnResult(BaseModel): def skill_success(self) -> bool: return self.skill_routing and self.output_present and self.no_error + def detail(self) -> dict: + """The subset of this result reported in detail["turns"] for one conversation turn.""" + return { + "turn_id": self.turn_id, + "expected_skill": self.expected_skill, + "skill_routing": self.skill_routing, + "output_present": self.output_present, + "output_correct": self.output_correct, + "activated_skills": self.activated_skills, + } + def _resolve_refs( expected_output: dict | None, @@ -426,35 +437,11 @@ def run_agentic_conversation( ) -@dataclass -class TurnDetail: - """The subset of a TurnResult reported in detail["turns"] for one conversation turn.""" - - turn_id: str - expected_skill: str - skill_routing: bool - output_present: bool - output_correct: bool | None - activated_skills: list[str] - - def _conversation_detail(result: ConversationResult) -> dict: return { "full_skill_coverage": result.full_skill_coverage, "total_clarification_turns": result.total_clarification_turns, - "turns": [ - asdict( - TurnDetail( - turn_id=tr.turn_id, - expected_skill=tr.expected_skill, - skill_routing=tr.skill_routing, - output_present=tr.output_present, - output_correct=tr.output_correct, - activated_skills=tr.activated_skills, - ) - ) - for tr in result.turn_results - ], + "turns": [tr.detail() for tr in result.turn_results], "latency_breakdown": build_latency_breakdown(result.tool_call_events, result.reasoning_step_events), } From 9aa8d86efd97fc646fb6448eccbc80886563afb1 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 3 Sep 2026 11:14:36 +0200 Subject: [PATCH 3/4] fix(gooddata-eval): return a copy of activated_skills from TurnResult.detail() detail() returned self.activated_skills directly -- a caller mutating the returned dict could mutate the TurnResult it came from. Caught by CodeRabbit on PR #1757. --- .../core/agentic/conversation.py | 2 +- .../tests/test_agentic_conversation.py | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) 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 f409593a9..a5619a88c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -78,7 +78,7 @@ def detail(self) -> dict: "skill_routing": self.skill_routing, "output_present": self.output_present, "output_correct": self.output_correct, - "activated_skills": self.activated_skills, + "activated_skills": list(self.activated_skills), } diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index f28deb124..82d1a071c 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -97,6 +97,25 @@ def test_turn_result_skill_success(): assert r.skill_success is True +def test_turn_result_detail_returns_independent_activated_skills_list(): + """Regression: detail()'s activated_skills must be a copy -- a caller mutating the + returned dict must not be able to mutate the TurnResult it came from. + """ + r = TurnResult( + turn_id="t1", + expected_skill="visualization", + skill_routing=True, + output_present=True, + no_error=True, + activated_skills=["visualization"], + clarification_turns_used=0, + output_correct=None, + ) + d = r.detail() + d["activated_skills"].append("mutated") + assert r.activated_skills == ["visualization"] + + def test_resolve_refs_no_refs(): assert _resolve_refs({"key": "value"}, {}) == {"key": "value"} From 0aa2b77ee0c7f5dd5ae373c6cd6a0dd0b615125d Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Fri, 4 Sep 2026 09:20:56 +0200 Subject: [PATCH 4/4] refactor(gooddata-eval): derive TurnResult.detail() from the model, not a key list Addresses hkad98's review: TurnResult is already a pydantic BaseModel, so a hand-written dict literal of six of its own field names was a second copy to keep in sync. detail() now returns model_dump(include=_DETAIL_FIELDS). Two things fall out of it: - A field rename can no longer leave detail() emitting a stale key with no test failure -- include= raises on a name the model doesn't have, and a new test asserts _DETAIL_FIELDS is a subset of model_fields. - model_dump deep-copies activated_skills, so the explicit list() wrapper is no longer needed; the copy regression test still passes. Also renames the copy test to test_turn_result_detail_copies_activated_skills (the previous 71-character name restated its own docstring) and lifts the shared TurnResult construction into a helper. --- .../core/agentic/conversation.py | 24 ++++++++++++------- .../tests/test_agentic_conversation.py | 19 +++++++++++---- 2 files changed, 29 insertions(+), 14 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 a5619a88c..fadc22b52 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -6,7 +6,7 @@ import json import re from dataclasses import dataclass, field -from typing import Literal +from typing import ClassVar, Literal from gooddata_sdk import GoodDataSdk from pydantic import BaseModel @@ -70,16 +70,22 @@ class TurnResult(BaseModel): def skill_success(self) -> bool: return self.skill_routing and self.output_present and self.no_error + # Reported per turn in detail["turns"]. A name listed here that no longer exists on the + # model raises rather than silently emitting a stale key, which a hand-written dict + # literal of the same fields would not -- and model_dump deep-copies activated_skills, + # so a caller mutating the returned dict cannot reach back into this TurnResult. + _DETAIL_FIELDS: ClassVar[set[str]] = { + "turn_id", + "expected_skill", + "skill_routing", + "output_present", + "output_correct", + "activated_skills", + } + def detail(self) -> dict: """The subset of this result reported in detail["turns"] for one conversation turn.""" - return { - "turn_id": self.turn_id, - "expected_skill": self.expected_skill, - "skill_routing": self.skill_routing, - "output_present": self.output_present, - "output_correct": self.output_correct, - "activated_skills": list(self.activated_skills), - } + return self.model_dump(include=self._DETAIL_FIELDS) def _resolve_refs( diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 82d1a071c..4413ad86f 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -97,11 +97,8 @@ def test_turn_result_skill_success(): assert r.skill_success is True -def test_turn_result_detail_returns_independent_activated_skills_list(): - """Regression: detail()'s activated_skills must be a copy -- a caller mutating the - returned dict must not be able to mutate the TurnResult it came from. - """ - r = TurnResult( +def _turn_result() -> TurnResult: + return TurnResult( turn_id="t1", expected_skill="visualization", skill_routing=True, @@ -111,11 +108,23 @@ def test_turn_result_detail_returns_independent_activated_skills_list(): clarification_turns_used=0, output_correct=None, ) + + +def test_turn_result_detail_copies_activated_skills(): + """A caller mutating the returned dict must not reach back into the TurnResult.""" + r = _turn_result() d = r.detail() d["activated_skills"].append("mutated") assert r.activated_skills == ["visualization"] +def test_turn_result_detail_fields_all_exist_on_the_model(): + """_DETAIL_FIELDS is a hand-listed subset, so a renamed field must fail here rather + than silently drop a key from every report.""" + assert set(TurnResult.model_fields) >= TurnResult._DETAIL_FIELDS + assert set(_turn_result().detail()) == TurnResult._DETAIL_FIELDS + + def test_resolve_refs_no_refs(): assert _resolve_refs({"key": "value"}, {}) == {"key": "value"}