From 26f2813a355aca6e05855453c4ec29af446c51af Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 1 Oct 2026 09:33:52 +0700 Subject: [PATCH 1/5] fix(gooddata-eval): do not retry a turn that hit the iteration limit gen-ai reports a turn that ended without a final answer as a 502 whose error event carries a `reason` (max_iterations, max_tokens, content_filter). The 502 alone routed it to the transient retry, which re-sent the same question up to five times: the work was repeated, the question landed in a conversation's history twice, and a stall was hidden from every evaluator. These reasons now raise TurnIncompleteError, a ChatError that is not retried and keeps the partial result. A 502 without a reason, or with `unknown`, is still treated as transient. This changes behaviour for every eval kind, not only conversations: an item whose turn hits the iteration limit now fails instead of being retried. jira: QA-29448 risk: low --- .../src/gooddata_eval/core/chat/sse_client.py | 19 +++++++++++ .../gooddata-eval/tests/test_sse_client.py | 32 ++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index 2127c88e3..bf2d602b2 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -38,6 +38,8 @@ # server bugs. A genuinely deterministic 500 still terminates, just after the bounded backoff. _RETRYABLE_STATUS_CODES: frozenset[int] = frozenset({429, 500, 502, 503, 504}) _METADATA_SYNC_MARKER = "METADATA_SYNC_IN_PROGRESS" +# `reason` values gen-ai puts on the error event of a turn that finished without an answer. +_TURN_INCOMPLETE_REASONS: frozenset[str] = frozenset({"max_iterations", "max_tokens", "content_filter"}) # Stands in for the `id` a persisted visualization would carry, on the fallback path # where the agent's create_adhoc_visualization call failed and only its arguments survive. _ADHOC_VIZ_ID = "adhoc-visualization-not-persisted" @@ -72,17 +74,29 @@ def __init__( status_code: int | None = None, detail: str | None = None, partial_result: ChatResult | None = None, + reason: str | None = None, ) -> None: super().__init__(message) self.status_code = status_code self.detail = detail self.partial_result = partial_result + self.reason = reason class TransientChatError(ChatError): """Retryable transient error: gen-ai temporarily unavailable or still syncing metadata.""" +class TurnIncompleteError(ChatError): + """The agent ended its turn without a final answer, e.g. at its iteration limit. + + gen-ai reports this as a 502 carrying a ``reason``, which the status code alone would + route to the transient retry. It is not transient: resending the question repeats the + same work, and on a conversation it also puts the question into the history twice. + Callers see it as the agent's own failure to answer. + """ + + class TurnTimeoutError(ChatError): """The agent exceeded a wall-clock budget -- either the per-turn one or the per-item one spanning every turn of a conversation. Not retryable: a slow turn stays slow, and @@ -383,6 +397,11 @@ def parse_sse_lines(lines: Iterable[str]) -> ChatResult: code = event_data.get("statusCode") detail = event_data.get("detail") message = f"SSE error {code}: {detail}" + reason = event_data.get("reason") + if reason in _TURN_INCOMPLETE_REASONS: + raise TurnIncompleteError( + message, status_code=code, detail=detail, partial_result=_build_chat_result(acc), reason=reason + ) if code in _RETRYABLE_STATUS_CODES: raise TransientChatError( message, status_code=code, detail=detail, partial_result=_build_chat_result(acc) diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index ae44fd9d1..250a8393f 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -4,7 +4,13 @@ import httpx import pytest from gooddata_eval.core.chat import sse_client as sse_mod -from gooddata_eval.core.chat.sse_client import ChatClient, ChatError, TransientChatError, parse_sse_lines +from gooddata_eval.core.chat.sse_client import ( + ChatClient, + ChatError, + TransientChatError, + TurnIncompleteError, + parse_sse_lines, +) from gooddata_eval.core.models import DatasetItem, ReasoningStepEvent, ToolCallEvent, build_latency_breakdown @@ -491,6 +497,30 @@ def test_parse_sse_lines_metadata_sync_marker_in_malformed_json_is_transient(): parse_sse_lines(["data: {bad json METADATA_SYNC_IN_PROGRESS"]) +@pytest.mark.parametrize("reason", ["max_iterations", "max_tokens", "content_filter"]) +def test_parse_sse_lines_turn_incomplete_is_not_transient(reason): + """gen-ai's "no final answer" error is a 502 with a reason; retrying it would resend the question.""" + lines = [ + 'data: {"item": {"role": "assistant", "content": {"type": "text", "text": "Let me check."}}}', + f'data: {{"statusCode": 502, "detail": "iteration limit", "reason": "{reason}"}}', + ] + with pytest.raises(TurnIncompleteError) as ei: + parse_sse_lines(lines) + assert not isinstance(ei.value, TransientChatError) + assert ei.value.reason == reason + assert ei.value.partial_result is not None + assert ei.value.partial_result.text_response == "Let me check." + + +@pytest.mark.parametrize("reason", [None, "unknown"]) +def test_parse_sse_lines_502_without_a_definite_reason_stays_transient(reason): + payload = {"statusCode": 502, "detail": "LLM provider is not responding"} + if reason: + payload["reason"] = reason + with pytest.raises(TransientChatError): + parse_sse_lines([f"data: {json.dumps(payload)}"]) + + def test_parse_sse_lines_non_retryable_status_is_chat_error_not_transient(): with pytest.raises(ChatError) as ei: parse_sse_lines(['data: {"statusCode": 400, "detail": "bad"}']) From 4a640945e116c646dfa08258be1eef16616831da Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 1 Oct 2026 09:33:52 +0700 Subject: [PATCH 2/5] fix(gooddata-eval): keep metrics the agent only updated during cleanup create_metric is an upsert: on an existing id it replaces that metric and reports `created_new: false`. Cleanup listed every create_metric result as created, so a run in which the agent updated a metric that was already in the workspace deleted it. _extract_created_metric_ids now skips results with created_new false. jira: QA-29448 risk: low --- .../gooddata_eval/core/agentic/metric_skill.py | 7 +++++-- .../tests/test_agentic_metric_skill.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) 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 275609e84..b0c2578e8 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 @@ -205,7 +205,8 @@ def _extract_created_metric_ids(tool_call_events: list[ToolCallEvent]) -> list[s """Ids of every metric created by ``create_metric`` calls (a turn may create more than one). Used for cleanup so no created metric leaks — unlike ``_extract_metric_result``, which - returns only the first result for MAQL evaluation. Shared with conversation evaluation. + returns only the first result for MAQL evaluation. ``create_metric`` is an upsert: a result + with ``created_new: false`` replaced a metric that existed before, so it is not listed. """ metric_ids: list[str] = [] for tc in tool_call_events: @@ -215,7 +216,9 @@ def _extract_created_metric_ids(tool_call_events: list[ToolCallEvent]) -> list[s if not result_data: continue data = result_data.get("data", result_data) - metric_id = data.get("metric_id") if isinstance(data, dict) else None + if not isinstance(data, dict) or data.get("created_new") is False: + continue + metric_id = data.get("metric_id") if metric_id and metric_id not in metric_ids: metric_ids.append(metric_id) return metric_ids diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 73282e6f5..418c6e04e 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -1,5 +1,6 @@ # (C) 2026 GoodData Corporation. All rights reserved. # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise +import json import sys import types from contextlib import ExitStack, contextmanager @@ -12,6 +13,7 @@ MetricSkillAssertionError, SimulatedResponseError, _delete_metric, + _extract_created_metric_ids, _extract_metric_result, _no_where_clause_hint, evaluate_agentic_metric_skill, @@ -853,3 +855,18 @@ def _capture(_submit, _identity, **kwargs): # they are counts, and a float reads as though a fraction of a turn were possible. assert isinstance(scores["turns"], int) assert isinstance(scores["steps"], int) + + +def test_extract_created_metric_ids_skips_a_metric_updated_in_place(): + """create_metric is an upsert; deleting an updated metric would delete one that existed before.""" + + def call(mid, created_new): + return ToolCallEvent.model_validate( + { + "functionName": "create_metric", + "functionArguments": "{}", + "result": json.dumps({"data": {"metric_id": mid, "created_new": created_new}}), + } + ) + + assert _extract_created_metric_ids([call("new", True), call("old", False)]) == ["new"] From f9b198eaddccbeec7de7fbc4271c9a9dd44e5270 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 1 Oct 2026 09:33:52 +0700 Subject: [PATCH 3/5] feat(gooddata-eval): grade conversations on keeping context A conversation passed when every turn had the expected skill active and produced an output of the expected type. Nothing compared what the output contained (the check that tried returned None for every chart in both conversation datasets), the simulated user was primed with the expected chart or MAQL and repaired lost context silently, and a stall was pushed past by the next simulated reply. A `context` mode adds, per turn: - The output must match the expected output: metrics, dimensions and every filter category for charts, MAQL for metrics, a subset of the arguments for tool calls. Alternatives are accepted; an end date after today counts as today; a single-value IN equals `=`. - A question back is graded by a binary judge (temperature 0, named boolean verdict): did it ask for information already in the conversation? Confirmation requests are not judged. - A reply that neither delivers nor asks is a stall; the user pushes once, then the turn ends. - The simulated user answers from the fixture's set answers and the conversation only, never from the expected output. Both modes compute every score; the mode (argument or GD_EVAL_CONVERSATION_MODE, default legacy) picks which verdict raises and is written as gate_passed. New scores: context_success, context_kept_rate, turns_before_first_break, lost_context_clarifications, stalled_turns. Fixtures may add depends_on, set_answers, expected_output_alternatives and expected_tool_args. Cleanup also deletes the alerts a conversation created, keeps metrics it only updated, and records objects from a stream that died mid-turn. fresh_conversation_per_turn runs a no-memory baseline for calibration. jira: QA-29448 risk: low --- .../core/agentic/_conversation_context.py | 295 ++++++++ .../core/agentic/conversation.py | 644 +++++++++++++++--- .../tests/test_agentic_conversation.py | 569 +++++++++++++++- 3 files changed, 1422 insertions(+), 86 deletions(-) create mode 100644 packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py new file mode 100644 index 000000000..665d3681c --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py @@ -0,0 +1,295 @@ +# (C) 2026 GoodData Corporation. All rights reserved. +"""What the conversation evaluator needs to judge context, not just output presence. + +Three questions get asked of every assistant reply that did not produce the turn's +expected output: + +1. What kind of reply is it -- a confirmation request ("Should I create this metric?"), + a question to the user, or a turn that ended without doing anything (a stall)? +2. For a question: did the assistant ask for something the conversation had already + established? That is the lost-context signal, graded by a binary LLM judge. +3. What does the simulated user say back? In ``context`` mode it never sees the turn's + expected output -- only the fixture's set answers and the conversation itself -- so a + lost context can no longer be repaired silently by a user who knows the answer key. + +Only information that is IN the conversation counts. A question about something the +assistant could have looked up in the workspace, but that nobody mentioned, is graded as +legitimate: whether the assistant should have looked it up is a separate check. +""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +from dataclasses import dataclass +from typing import Literal + +from gooddata_eval.core.config import judge_model +from gooddata_eval.core.evaluators._llm_judge import JudgeResponseError, LLMJudge, _message_content +from gooddata_eval.core.models import ChatResult + +ReplyKind = Literal["confirmation", "question", "no_action"] + +NUDGE_MESSAGE = "Please go ahead and do it." +CONFIRMATION_REPLY = "Yes, go ahead." +# Asked to pick from a list (which dashboard to bind an alert to), a real user picks one. +NO_ANSWER_REPLY = "Use your best judgement and go ahead. If you need me to pick from a list, take the first option." + +# Part of the metric and alert skills' designed flow, not a clarification. +_CONFIRMATION_RE = re.compile( + r"\b(?:should|shall|may|can)\s+i\s+(?:go\s+ahead|proceed|create|save|update|apply|add|set\s+(?:it|this)\s+up)" + r"|\bdo\s+you\s+want\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)" + r"|\bwould\s+you\s+like\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)" + r"|\bplease\s+confirm\b|\bconfirm\s+(?:if|whether|that)\b", + re.IGNORECASE, +) + +# Checked after _CONFIRMATION_RE, so "please confirm" stays a confirmation. +_REQUEST_RE = re.compile( + r"\bplease\s+(?:select|choose|pick|specify|provide|tell\s+me|let\s+me\s+know|clarify|share)\b" + r"|\breply\s+with\b|\b(?:select|choose|pick)\s+(?:one|a|an|the)\b", + re.IGNORECASE, +) + +_MESSAGE_CHARS = 800 +_TRANSCRIPT_MESSAGES = 40 + + +def has_structured_question(chat_result: ChatResult) -> bool: + """gen-ai's own signal that the reply is a question: a ``clarifyingQuestions`` part.""" + return any(part.get("type") == "clarifyingQuestions" for part in chat_result.unhandled_parts or []) + + +def classify_reply(chat_result: ChatResult, rendered_text: str) -> ReplyKind: + """Kind of a reply that did not produce the expected output. + + The structured ``clarifyingQuestions`` part is authoritative when present. Without it + the text decides: a confirmation phrase or an alert proposal is a confirmation, a + question mark is a question, and anything else -- including an empty reply -- is a turn + that stopped without acting. + """ + if has_structured_question(chat_result): + return "question" + if chat_result.alert_proposals: + return "confirmation" + text = (rendered_text or "").strip() + if not text: + return "no_action" + if _CONFIRMATION_RE.search(text): + return "confirmation" + if "?" in text or _REQUEST_RE.search(text): + return "question" + return "no_action" + + +@dataclass +class TranscriptEntry: + role: Literal["user", "assistant"] + text: str + + +def clip(text: str, limit: int = _MESSAGE_CHARS) -> str: + text = " ".join((text or "").split()) + return text if len(text) <= limit else text[: limit - 1] + "…" + + +def summarize_visualizations(chat_result: ChatResult) -> str: + """One line per chart the reply showed: what it plotted and how it was filtered. + + Charts carry most of what a conversation establishes ("the Apparel filter", "by + quarter"), and they are invisible in the text part. The judge needs them to know what + was already settled. + """ + vizzes = chat_result.created_visualizations + objects = getattr(vizzes, "objects", None) or [] + lines = [] + for viz in objects: + fields = [getattr(f, "using", f) for f in (viz.query.fields or {}).values()] + filters = [ + {k: v for k, v in f.items() if k in ("type", "using", "state", "from", "to", "top", "bottom")} + for f in (viz.query.filter_by or {}).values() + if isinstance(f, dict) + ] + lines.append(f"[chart {viz.type or ''}: fields={fields} filters={filters}]") + return " ".join(lines) + + +def render_transcript(entries: list[TranscriptEntry]) -> str: + recent = entries[-_TRANSCRIPT_MESSAGES:] + return "\n".join(f"{e.role.upper()}: {clip(e.text)}" for e in recent) + + +_JUDGE_SYSTEM = """\ +You grade one question that an analytics assistant asked its user. You get: +- CONVERSATION BEFORE: everything said before the user's latest message (may be empty), +- LATEST MESSAGE: what the user just asked, +- QUESTION: the assistant's reply, in which it asks the user something instead of doing it. + +Decide whether the information the QUESTION asks for already appears in CONVERSATION BEFORE: +stated by the user, or determined by an assistant answer -- for example which metric was meant, +which filters apply, which item came out weakest or biggest, what a chart showed, which object +was created. + +Rules: +- The LATEST MESSAGE does not count. A reference in it ("it", "that second one", "those + brands") only points at something that must appear in CONVERSATION BEFORE. +- If CONVERSATION BEFORE is empty, the answer is false. +- Information that exists in the workspace but was never mentioned in the conversation does not + count. +- Do not judge whether asking was useful, necessary or well phrased. + +Examples: +- BEFORE: user "Net sales by country." / assistant "Here is net sales by country." LATEST: "Add + order count to it." QUESTION: "Which chart should I add order count to?" -> true (the chart + just made). +- BEFORE: user "Top 10 products by revenue." / assistant "Here are the top 10 products." + LATEST: "Remove the limit." QUESTION: "Which limit do you mean?" -> true (the top 10). +- BEFORE: (empty) LATEST: "Show revenue by month." QUESTION: "Which revenue metric: gross or + net?" -> false (nothing earlier says which). + +Return a JSON object with exactly two keys: + "already_in_conversation": true or false + "reasoning": one sentence naming the information and where it appears, or that it does not +""" + +_JUDGE_USER = "CONVERSATION BEFORE:\n{history}\n\nLATEST MESSAGE:\n{latest}\n\nQUESTION:\n{question}" + + +def judge_prompt(history: str, latest: str, question: str) -> str: + """The user prompt the clarification judge reads, also recorded for auditing.""" + return _JUDGE_USER.format(history=history or "(empty)", latest=latest, question=question) + + +@dataclass(frozen=True) +class ClarificationVerdict: + lost_context: bool | None # None when the judge could not be run or gave no verdict + reasoning: str + error: str | None = None + + +class _ClarificationLLM(LLMJudge): + """``LLMJudge``'s client, temperature handling and retry, with this judge's own prompt and + verdict key. The key names the question asked, so a 0/1 cannot be read the wrong way round.""" + + def __init__(self, model: str | None = None) -> None: + super().__init__([], model=model) + self._system_prompt = _JUDGE_SYSTEM + + def already_in_conversation(self, prompt: str) -> tuple[bool, str]: + messages = [{"role": "system", "content": self._system_prompt}, {"role": "user", "content": prompt}] + raw = None + for _attempt in range(2): + raw = _message_content(self._create_completion(messages)) + if isinstance(raw, str) and raw.strip(): + break + if not (isinstance(raw, str) and raw.strip()): + raise JudgeResponseError(f"clarification judge {self.model!r} returned an empty body twice") + try: + data = json.loads(raw) + except json.JSONDecodeError as exc: + raise JudgeResponseError(f"clarification judge returned unparseable JSON: {raw!r}") from exc + value = data.get("already_in_conversation") if isinstance(data, dict) else None + if not isinstance(value, bool): + raise JudgeResponseError(f"clarification judge returned no boolean verdict: {raw!r}") + return value, str(data.get("reasoning", "")) + + +class ClarificationJudge: + """Binary judge: did the assistant ask for something the conversation already established? + + Temperature 0 and a JSON verdict, on ``LLMJudge``'s client. Verdicts are cached per prompt, + so a conversation that repeats the same question does not pay for it twice. + """ + + def __init__(self, model: str | None = None) -> None: + self._model = model + self._llm: _ClarificationLLM | None = None + self._cache: dict[str, ClarificationVerdict] = {} + self._warned = False + + @property + def model_name(self) -> str: + """The judge model, without building a client (which needs an API key).""" + return self._llm.model if self._llm is not None else (self._model or judge_model()) + + def judge(self, history: str, latest: str, question: str) -> ClarificationVerdict: + prompt = judge_prompt(history, latest, question) + key = hashlib.sha256(prompt.encode()).hexdigest() + if key in self._cache: + return self._cache[key] + try: + if self._llm is None: + self._llm = _ClarificationLLM(self._model) + try: + found, reasoning = self._llm.already_in_conversation(prompt) + verdict = ClarificationVerdict(lost_context=found, reasoning=reasoning) + except JudgeResponseError as exc: + verdict = ClarificationVerdict(lost_context=None, reasoning="", error=str(exc)) + except Exception as exc: # noqa: BLE001 -- a grading fault must not fail the agent's conversation + if not self._warned: + print(f"[JUDGE] clarifications left unjudged: {exc}") + self._warned = True + verdict = ClarificationVerdict(lost_context=None, reasoning="", error=str(exc)) + self._cache[key] = verdict + return verdict + + +_SIM_USER_MODEL = "gpt-4o" + + +def _chat(prompt_system: str, prompt_user: str) -> str | None: + try: + from openai import OpenAI # noqa: PLC0415 + except ImportError: + return None + api_key = os.environ.get("OPENAI_API_KEY") + if not api_key: + return None + try: + response = OpenAI(api_key=api_key).chat.completions.create( + model=_SIM_USER_MODEL, + messages=[{"role": "system", "content": prompt_system}, {"role": "user", "content": prompt_user}], + temperature=0, + max_tokens=200, + ) + except Exception as exc: + print(f"[SIM-USER] reply failed: {exc}") + return None + content = response.choices[0].message.content if response.choices else None + return content.strip() if content else None + + +def reply_from_facts(transcript: str, question: str, facts: list[str]) -> str: + """A legitimate question the fixture has no unused set answer for. + + The reply may use only the fixture's facts for this turn -- never the expected output -- + and falls back to "use your best judgement" when the facts do not cover the question. + """ + if not facts: + return NO_ANSWER_REPLY + reply = _chat( + "You play a business user talking to an analytics assistant. Answer its question briefly, " + "using ONLY the facts listed below. Never invent requirements. If the facts do not answer " + f"the question, reply exactly: {NO_ANSWER_REPLY}", + f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}\n\n" + "Facts you know:\n" + "\n".join(f"- {f}" for f in facts), + ) + return reply or NO_ANSWER_REPLY + + +def reply_restating(transcript: str, question: str) -> str: + """The assistant asked for something the conversation already established. + + The turn is already failed; restating keeps the rest of the conversation measurable, + which is what a real user would do too. + """ + reply = _chat( + "You play a business user talking to an analytics assistant. The assistant just asked for " + "something that was already said or found earlier in the conversation. Reply briefly, " + "starting with 'As I said earlier,' and repeat only that information, taken from the " + "conversation. Do not add anything new.", + f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}", + ) + return reply or "As I said earlier in this conversation — please use what we already established." 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 98a5066ed..5385c644b 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -1,16 +1,45 @@ # (C) 2026 GoodData Corporation. All rights reserved. -"""Agentic conversation evaluation runner (multi-turn, multi-skill).""" +"""Agentic conversation evaluation runner (multi-turn, multi-skill). + +Two modes decide what a pass means: + +- ``legacy``: a turn passes when the expected skill is active and an output of the + expected type appears. The simulated user is primed with the turn's expected output. +- ``context``: a turn additionally has to match its expected output given everything the + conversation established, must not ask for information the conversation already holds + (graded by a binary judge), and must not stall. The simulated user answers from the + fixture's set answers and the conversation only, never from the expected output. + +Both modes compute every score, so a dataset can run in ``legacy`` while the ``context`` +scores are collected alongside it; the mode only picks which verdict raises. +""" from __future__ import annotations import json +import os import re from dataclasses import dataclass, field +from datetime import date from typing import ClassVar, Literal from gooddata_sdk import GoodDataSdk -from pydantic import BaseModel, Field - +from pydantic import BaseModel, Field, ValidationError + +from gooddata_eval.core.agentic._conversation_context import ( + CONFIRMATION_REPLY, + NUDGE_MESSAGE, + ClarificationJudge, + TranscriptEntry, + classify_reply, + clip, + judge_prompt, + render_transcript, + reply_from_facts, + reply_restating, + summarize_visualizations, +) +from gooddata_eval.core.agentic._gate import log_gate_scores from gooddata_eval.core.agentic._trace_linker import ( RunIdentity, RunTraceContext, @@ -20,15 +49,18 @@ submit_trace_scoring, utc_now, ) -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.agentic.alert_skill import _delete_alert, render_alert_proposal +from gooddata_eval.core.agentic.metric_skill import _delete_metric, _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, TurnIncompleteError from gooddata_eval.core.config import ReasoningEffort +from gooddata_eval.core.evaluators._deep_subset import deep_subset +from gooddata_eval.core.evaluators._maql import normalize_maql from gooddata_eval.core.models import ( AgenticAssertionError, AgenticEvalOutcome, ChatResult, + CreatedVisualization, ReasoningStepEvent, ToolCallEvent, shift_and_index_events, @@ -39,15 +71,35 @@ check_viz_type, get_dimension_uri_set, get_metric_uri_set, + normalized_filters, + resolve_alias_to_uri, ) _REF_PATTERN = re.compile(r"\$ref:([\w_]+)\.([\w_]+)") _DEFAULT_MAX_CLARIFICATION_TURNS = 7 +ConversationMode = Literal["legacy", "context"] +CONVERSATION_MODE_ENV_VAR = "GD_EVAL_CONVERSATION_MODE" + + +def resolve_conversation_mode(mode: str | None = None) -> ConversationMode: + """The explicit mode, else ``GD_EVAL_CONVERSATION_MODE``, else ``legacy``.""" + raw = (mode or os.environ.get(CONVERSATION_MODE_ENV_VAR, "") or "legacy").strip().lower() + if raw not in ("legacy", "context"): + raise ValueError(f"conversation mode must be 'legacy' or 'context', got {raw!r}") + return "context" if raw == "context" else "legacy" + class TurnDefinition(BaseModel): - """Definition of a single turn in a multi-turn conversation evaluation.""" + """Definition of a single turn in a multi-turn conversation evaluation. + + ``depends_on`` names the earlier turns this one cannot be answered without. It does not + change how the turn is graded; it selects which turns ``context_kept_rate`` is computed + over. ``set_answers`` are what the user replies, in order, when the assistant asks a + legitimate question on this turn -- the only knowledge the context-mode simulated user + has beyond the conversation itself. + """ turn_id: str message: str @@ -55,6 +107,10 @@ class TurnDefinition(BaseModel): expected_output_type: Literal["visualization", "tool_call", "metric"] = "visualization" expected_tool_name: str | None = None expected_output: dict | None = None + expected_output_alternatives: list[dict] = Field(default_factory=list) + expected_tool_args: dict | None = None + depends_on: list[str] = Field(default_factory=list) + set_answers: list[str] = Field(default_factory=list) class ConversationFixture(BaseModel): @@ -66,6 +122,21 @@ class ConversationFixture(BaseModel): turns: list[TurnDefinition] +ReplyRecordKind = Literal["confirmation", "question", "no_action", "turn_incomplete"] + + +class ClarificationRecord(BaseModel): + """One assistant reply that did not deliver the turn's output, and what the user said back.""" + + kind: ReplyRecordKind + agent_message: str + lost_context: bool | None = None + reasoning: str = "" + judge_error: str | None = None + judge_input: str = "" + reply: str = "" + + class TurnResult(BaseModel): """Evaluation result for a single conversation turn. @@ -81,6 +152,10 @@ class TurnResult(BaseModel): ``skill_routing=True`` with ``activated_skills=[]`` is the reused-skill case, not a scoring bug -- ``active_skills`` shows where the credit came from. + + ``skill_success`` is the legacy verdict. ``context_success`` also requires the output to + match the expected one (when the fixture states it), no clarification the judge found + to be lost context, and no stall. """ turn_id: str @@ -93,11 +168,43 @@ class TurnResult(BaseModel): active_skills: list[str] = Field(default_factory=list) clarification_turns_used: int = 0 output_correct: bool | None = None + mismatches: list[str] = Field(default_factory=list) + clarifications: list[ClarificationRecord] = Field(default_factory=list) + depends_on: list[str] = Field(default_factory=list) @property def skill_success(self) -> bool: return self.skill_routing and self.output_present and self.no_error + @property + def lost_context(self) -> bool: + return any(c.lost_context is True for c in self.clarifications) + + @property + def stalled(self) -> bool: + return any(c.kind in ("no_action", "turn_incomplete") for c in self.clarifications) + + @property + def context_success(self) -> bool: + return self.skill_success and self.output_correct is not False and not self.lost_context and not self.stalled + + def failure_reasons(self) -> list[str]: + """Why ``context_success`` is False, in reading order; empty when it is True.""" + reasons: list[str] = [] + if not self.no_error: + reasons.append("turn could not run") + elif not self.output_present: + reasons.append("no output of the expected type") + elif not self.skill_routing: + reasons.append(f"skill {self.expected_skill!r} not active") + if self.output_correct is False: + reasons.extend(self.mismatches or ["output does not match the expected output"]) + if self.lost_context: + reasons.append("asked for information the conversation already established") + if self.stalled: + reasons.append("ended a reply without the expected output and without asking anything") + return reasons + # 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, @@ -112,11 +219,18 @@ 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", + "clarification_turns_used", + "mismatches", + "clarifications", + "depends_on", } def detail(self) -> dict: """The subset of this result reported in detail["turns"] for one conversation turn.""" - return self.model_dump(include=self._DETAIL_FIELDS) + out = self.model_dump(include=self._DETAIL_FIELDS) + out["context_success"] = self.context_success + out["failure_reasons"] = self.failure_reasons() + return out def _resolve_refs( @@ -210,56 +324,170 @@ def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool return False -def _check_output_correct(turn: TurnDefinition, chat_result: ChatResult) -> bool | None: - """Check output correctness against expected_output when defined. +def _expected_viz(expected: dict) -> CreatedVisualization | None: + """The chart an expected output describes, or None when it names no fields to compare. - Returns None when expected_output is absent (presence check only). + Fixtures wrap the chart as ``{"visualization": {...}}``; a bare chart is accepted too. + ``type`` may be empty, which means "any chart type". """ - from gooddata_eval.core.evaluators._maql import normalize_maql # noqa: PLC0415 + body = expected.get("visualization", expected) + if not isinstance(body, dict): + return None + query = body.get("query") + if not isinstance(query, dict) or not query.get("fields"): + return None + try: + return CreatedVisualization.model_validate({**body, "type": body.get("type") or ""}) + except ValidationError: + return None + + +def _flat_viz_check(candidates: list[dict], chat_result: ChatResult) -> tuple[bool | None, list[str]]: + """The flat ``{"metrics": [uri, ...], "dimensions": [uri, ...]}`` form: the listed URIs + must all be in one of the charts. ``(None, [])`` when no candidate lists any.""" + flat = [c for c in candidates if isinstance(c.get("metrics"), list) or isinstance(c.get("dimensions"), list)] + if not flat: + return None, [] + objects = getattr(chat_result.created_visualizations, "objects", None) or [] + for cand in flat: + want_m, want_d = set(cand.get("metrics") or []), set(cand.get("dimensions") or []) + for act in objects: + if want_m <= get_metric_uri_set(act) and want_d <= get_dimension_uri_set(act): + return True, [] + return False, [f"expected {flat[0].get('metrics') or []} / {flat[0].get('dimensions') or []} in the chart"] + + +def _sort_signature(viz: CreatedVisualization) -> set[tuple[str, str, str]]: + """(sort type, sorted-by URI, direction) for each sort entry.""" + out: set[tuple[str, str, str]] = set() + for entry in viz.query.sort_by: + if not isinstance(entry, dict): + continue + stype = str(entry.get("type") or "") + direction = str(entry.get("direction") or "").upper() + aliases = entry.get("metrics") if stype == "metric_sort" else [entry.get("by")] + for alias in aliases or []: + if isinstance(alias, str): + out.add((stype, resolve_alias_to_uri(alias, viz.query.fields), direction)) + return out + +def _clamp_future_dates(viz: CreatedVisualization, today: date) -> CreatedVisualization: + """A copy whose absolute date filters end no later than ``today``. + + Nothing is recorded after today, so "2025 and 2026 so far" as 2025-01-01..2026-12-31 and as + 2025-01-01.. select the same data. Comparing the raw end dates would fail an answer + that states the period more precisely than the fixture does. + """ + filters = {} + for key, f in viz.query.filter_by.items(): + to = f.get("to") if isinstance(f, dict) and f.get("type") == "date_filter" else None + if isinstance(to, str) and re.fullmatch(r"\d{4}-\d{2}-\d{2}", to) and to > today.isoformat(): + f = {**f, "to": today.isoformat()} + filters[key] = f + return viz.model_copy(update={"query": viz.query.model_copy(update={"filter_by": filters})}) + + +def _viz_mismatches( + expected: CreatedVisualization, actual: CreatedVisualization, today: date | None = None +) -> list[str]: + """Every way ``actual`` differs from ``expected``; empty when they match. + + Metrics and dimensions compare as sets of URIs, so bucket order and local ids do not + matter. Filters compare per category and always: a chart with a filter nobody asked for + fails as surely as one missing a filter. Type and sort compare only when the expected + chart states them. + """ + today = today or date.today() + expected, actual = _clamp_future_dates(expected, today), _clamp_future_dates(actual, today) + out: list[str] = [] + exp_m, act_m = get_metric_uri_set(expected), get_metric_uri_set(actual) + if exp_m != act_m: + out.append(f"metrics: expected {sorted(exp_m)}, got {sorted(act_m)}") + exp_d, act_d = get_dimension_uri_set(expected), get_dimension_uri_set(actual) + if exp_d != act_d: + out.append(f"dimensions: expected {sorted(exp_d)}, got {sorted(act_d)}") + if not check_viz_type(expected, actual): + out.append(f"type: expected {expected.type!r}, got {actual.type!r}") + scores = check_filters(expected, actual, today) + exp_f, act_f = normalized_filters(expected, today), normalized_filters(actual, today) + for category, ok in (("date", scores.date_ok), ("ranking", scores.ranking_ok), ("attribute", scores.attribute_ok)): + if not ok: + out.append(f"{category} filters: expected {exp_f[category]}, got {act_f[category]}") + if expected.query.sort_by: + exp_s, act_s = _sort_signature(expected), _sort_signature(actual) + # An expected entry without a direction matches either direction. + if not all(any(a[:2] == e[:2] and (not e[2] or a[2] == e[2]) for a in act_s) for e in exp_s): + out.append(f"sort: expected {sorted(exp_s)}, got {sorted(act_s)}") + return out + + +# `IN ("x")` with one value is `= "x"`: both spellings appear in agent output for the same filter. +_SINGLE_IN_RE = re.compile(r'(? str: + return _SINGLE_IN_RE.sub(r"=\1", normalize_maql(maql)) + + +def _tool_calls_named(tool_call_events: list[ToolCallEvent], name: str | None) -> list[ToolCallEvent]: + return [tc for tc in tool_call_events if name is None or tc.function_name == name] + + +def _check_output_correct( + turn: TurnDefinition, chat_result: ChatResult, turn_tool_calls: list[ToolCallEvent] | None = None +) -> tuple[bool | None, list[str]]: + """Whether the turn's output matches its expected output, and how it differs if not. + + Returns ``(None, [])`` when the fixture states nothing to compare. The expected output + and each of ``expected_output_alternatives`` are candidates; matching any one is a pass. + ``turn_tool_calls`` are every tool call of the turn, across its clarification rounds -- + the tool whose arguments are checked may have run before the final reply. + """ + tool_calls = turn_tool_calls if turn_tool_calls is not None else (chat_result.tool_call_events or []) + candidates = [c for c in [turn.expected_output, *turn.expected_output_alternatives] if c] otype = turn.expected_output_type - expected = turn.expected_output - if not expected: - return None if otype == "visualization": - from gooddata_eval.core.models import CreatedVisualization # noqa: PLC0415 - - vizzes = chat_result.created_visualizations - if not vizzes: - return False - objects = getattr(vizzes, "objects", None) + expected_vizzes = [v for v in (_expected_viz(c) for c in candidates) if v is not None] + if not expected_vizzes: + return _flat_viz_check(candidates, chat_result) + objects = getattr(chat_result.created_visualizations, "objects", None) or [] if not objects: - return False - viz = objects[0] - results: list[bool] = [] - if "viz_type" in expected or "type" in expected: - try: - exp_viz = CreatedVisualization.model_validate(expected.get("visualization", expected)) - results.append(check_viz_type(exp_viz, viz)) - except Exception: - pass - if expected.get("metrics"): - actual_uris = get_metric_uri_set(viz) - results.append(all(m in actual_uris for m in expected["metrics"])) - if expected.get("dimensions"): - actual_uris = get_dimension_uri_set(viz) - results.append(all(d in actual_uris for d in expected["dimensions"])) - if "filters" in expected: - try: - exp_viz = CreatedVisualization.model_validate(expected.get("visualization", expected)) - results.append(check_filters(exp_viz, viz).all_ok) - except Exception: - pass - return all(results) if results else None + return False, ["no visualization to compare"] + best: list[str] | None = None + for exp in expected_vizzes: + for act in objects: + diff = _viz_mismatches(exp, act) + if not diff: + return True, [] + if best is None or len(diff) < len(best): + best = diff + return False, best or [] if otype == "metric": - metric_result = _extract_metric_result(chat_result.tool_call_events or []) + maqls: list[str] = [m for m in (c.get("maql") for c in candidates) if isinstance(m, str) and m] + if not maqls: + return None, [] + metric_result = _extract_metric_result(tool_calls) if not metric_result: - return False - return normalize_maql(metric_result.get("maql", "")) == normalize_maql(expected.get("maql", "")) + return False, ["no created metric to compare"] + actual = _canonical_maql(metric_result.get("maql", "")) + if any(_canonical_maql(m) == actual for m in maqls): + return True, [] + return False, [f"maql: expected {maqls[0]!r}, got {metric_result.get('maql', '')!r}"] + + if otype == "tool_call": + if not turn.expected_tool_args: + return None, [] + calls = _tool_calls_named(tool_calls, turn.expected_tool_name) + args = [tc.parsed_arguments() or {} for tc in calls] + if any(deep_subset(turn.expected_tool_args, a) for a in args): + return True, [] + got = args[-1] if args else None + return False, [f"tool arguments: expected subset {turn.expected_tool_args}, got {got}"] - return None + return None, [] def _get_sim_user_response(agent_message: str, turn: TurnDefinition, expected_output: dict | None) -> str: @@ -329,7 +557,14 @@ def _get_sim_user_response(agent_message: str, turn: TurnDefinition, expected_ou @dataclass class ConversationResult: - """Outcome of a multi-turn, multi-skill conversation evaluation.""" + """Outcome of a multi-turn, multi-skill conversation evaluation. + + ``conversation_success`` is the legacy verdict, ``context_success`` the context one; see + the module docstring. ``context_kept_rate`` is the share of context-dependent turns + (those with ``depends_on``, else every turn after the first) that passed in context + terms, and ``turns_before_first_break`` how many turns passed before the first one that + did not. + """ conversation_id: str turn_results: list[TurnResult] @@ -341,6 +576,102 @@ class ConversationResult: response_id: str | None = None tool_call_events: list[ToolCallEvent] = field(default_factory=list) reasoning_step_events: list[ReasoningStepEvent] = field(default_factory=list) + context_success: bool = False + context_kept_rate: float | None = None + turns_before_first_break: int = 0 + lost_context_clarifications: int = 0 + stalled_turns: int = 0 + mode: ConversationMode = "legacy" + judge_model: str | None = None + + +def _metric_creations(tool_call_events: list[ToolCallEvent]) -> list[tuple[str, bool]]: + """(metric_id, created_new) for every successful create_metric call. + + ``create_metric`` is an upsert: on an existing id it replaces that metric and reports + ``created_new: false``. A result without the flag counts as a creation. + """ + out: list[tuple[str, bool]] = [] + for tc in 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 + data = result_data.get("data", result_data) + if not isinstance(data, dict) or data.get("isError"): + continue + metric_id = data.get("metric_id") + if metric_id: + out.append((metric_id, data.get("created_new") is not False)) + return out + + +def _created_alert_ids(tool_call_events: list[ToolCallEvent]) -> list[str]: + """Ids of the alerts (automations) ``create_metric_alert`` calls created.""" + ids: list[str] = [] + for tc in tool_call_events: + if tc.function_name != "create_metric_alert" or not tc.result: + continue + result_data = tc.parsed_result() + if not isinstance(result_data, dict): + continue + nested = result_data.get("data") + alert_id = result_data.get("id") or (nested.get("id") if isinstance(nested, dict) else None) + if isinstance(alert_id, str) and alert_id and alert_id not in ids: + ids.append(alert_id) + return ids + + +class _WorkspaceObjects: + """What a conversation wrote to the workspace, for cleanup once it ends. + + Deferred to the end because a later turn may ``$ref`` a metric an earlier turn created. + A metric the agent updated in place (``created_new: false``) existed before the + conversation, so it is reported rather than deleted. + """ + + def __init__(self) -> None: + self.metrics: list[str] = [] + self.updated_metrics: list[str] = [] + self.alerts: list[str] = [] + + def record(self, tool_call_events: list[ToolCallEvent]) -> None: + for metric_id, created_new in _metric_creations(tool_call_events): + if created_new: + if metric_id not in self.metrics and metric_id not in self.updated_metrics: + self.metrics.append(metric_id) + elif metric_id not in self.metrics and metric_id not in self.updated_metrics: + self.updated_metrics.append(metric_id) + for alert_id in _created_alert_ids(tool_call_events): + if alert_id not in self.alerts: + self.alerts.append(alert_id) + + def delete(self, sdk: GoodDataSdk, workspace_id: str) -> None: + for alert_id in self.alerts: + _delete_alert(sdk, workspace_id, alert_id) + for metric_id in self.metrics: + _delete_metric(sdk, workspace_id, metric_id) + for metric_id in self.updated_metrics: + print(f"[CLEANUP] Metric {metric_id} existed before the conversation and was updated in place; kept.") + + +def _context_summary( + turn_results: list[TurnResult], fixture: ConversationFixture +) -> tuple[bool, float | None, int, int, int]: + """(context_success, context_kept_rate, turns_before_first_break, lost_context, stalled).""" + context_success = bool(turn_results) and all(tr.context_success for tr in turn_results) + labelled = {t.turn_id for t in fixture.turns if t.depends_on} + dependent = [tr for tr in turn_results if tr.turn_id in labelled] if labelled else turn_results[1:] + rate = (sum(tr.context_success for tr in dependent) / len(dependent)) if dependent else None + before_break = len(turn_results) + for i, tr in enumerate(turn_results): + if not tr.context_success: + before_break = i + break + lost = sum(1 for tr in turn_results for c in tr.clarifications if c.lost_context is True) + stalled = sum(1 for tr in turn_results if tr.stalled) + return context_success, rate, before_break, lost, stalled def run_agentic_conversation( @@ -352,13 +683,24 @@ def run_agentic_conversation( initial_conversation_id: str | None = None, reasoning_effort: ReasoningEffort | None = None, agent_id: str | None = None, + mode: ConversationMode | None = None, + clarification_judge: ClarificationJudge | None = None, + fresh_conversation_per_turn: bool = False, ) -> ConversationResult: """Run a multi-turn, multi-skill conversation evaluation (no K-runs). A single conversation is used for all turns in the fixture. Each turn may trigger up to *max_clarification_turns* additional rounds of simulated-user replies before the agent produces the expected output. + + ``fresh_conversation_per_turn`` sends every turn into a new conversation instead, so the + agent sees no history. It is the no-memory baseline a context score is calibrated + against: context-dependent turns are expected to fail there. """ + resolved_mode = resolve_conversation_mode(mode) + if fresh_conversation_per_turn and initial_conversation_id is not None: + raise ValueError("fresh_conversation_per_turn needs conversations this run creates itself") + judge = clarification_judge if clarification_judge is not None else ClarificationJudge() client = ChatClient( host=host, token=token, workspace_id=workspace_id, reasoning_effort=reasoning_effort, agent_id=agent_id ) @@ -369,14 +711,12 @@ def run_agentic_conversation( total_steps = 0 conversation_id: str = "" owns_conversation = False - # Metrics created during this conversation, deleted after it completes so they do - # not persist in the (shared) workspace and get reused by a later test. Deferred to - # the end — a later turn may $ref a metric an earlier turn created. - created_metric_ids: list[str] = [] + created = _WorkspaceObjects() reasoning_steps: list[str] = [] response_id: str | None = None conversation_tool_call_events: list[ToolCallEvent] = [] conversation_reasoning_step_events: list[ReasoningStepEvent] = [] + transcript: list[TranscriptEntry] = [] # The skills active right now, mirroring the platform's own state machine: set_skills # REPLACES the active set rather than adding to it -- verified against the gen-ai # service's skill registry, and stated in the tool's own description. So a turn that @@ -405,9 +745,18 @@ def run_agentic_conversation( conversation_id = client.create_conversation() owns_conversation = True - for turn in fixture.turns: + for turn_index, turn in enumerate(fixture.turns): + if fresh_conversation_per_turn and turn_index > 0: + client.delete_conversation(conversation_id) + conversation_id = client.create_conversation() + transcript = [] + active_skills = set() try: resolved_expected = _resolve_refs(turn.expected_output, turn_outputs) + resolved_alternatives = [ + _resolve_refs(a, turn_outputs) or {} for a in turn.expected_output_alternatives + ] + resolved_tool_args = _resolve_refs(turn.expected_tool_args, turn_outputs) except ValueError as exc: print(f"[SKIP] turn '{turn.turn_id}': {exc}") turn_results.append( @@ -423,18 +772,40 @@ def run_agentic_conversation( # read as "nothing was active", which is a different claim. active_skills=sorted(active_skills), output_correct=False, + mismatches=[str(exc)], + depends_on=list(turn.depends_on), ) ) continue - resolved_turn = turn.model_copy(update={"expected_output": resolved_expected}) + resolved_turn = turn.model_copy( + update={ + "expected_output": resolved_expected, + "expected_output_alternatives": resolved_alternatives, + "expected_tool_args": resolved_tool_args, + } + ) clarification_turns = 0 all_tool_calls: list[ToolCallEvent] = [] current_message = turn.message final_result: ChatResult | None = None + records: list[ClarificationRecord] = [] + answers_left = list(turn.set_answers) for _iter in range(max_clarification_turns + 1): - chat_result = client.send_message(conversation_id, current_message) + transcript.append(TranscriptEntry("user", current_message)) + incomplete = False + try: + chat_result = client.send_message(conversation_id, current_message) + except TurnIncompleteError as exc: + chat_result = exc.partial_result or ChatResult() + incomplete = True + except ChatError as exc: + # The stream can die after create_metric ran server-side; keep what the + # accumulator saw so the cleanup still knows what to delete. + if exc.partial_result is not None: + created.record(exc.partial_result.tool_call_events or []) + raise final_result = chat_result total_steps += chat_result.reasoning_step_count turn_offset, tool_index_offset, reasoning_index_offset = shift_and_index_events( @@ -444,26 +815,75 @@ def run_agentic_conversation( reasoning_index_offset=reasoning_index_offset, ) all_tool_calls.extend(chat_result.tool_call_events or []) + created.record(chat_result.tool_call_events or []) conversation_tool_call_events.extend(chat_result.tool_call_events or []) conversation_reasoning_step_events.extend(chat_result.reasoning_step_events or []) reasoning_steps.extend(chat_result.reasoning_steps or []) response_id = chat_result.response_id or response_id - if _check_output_present(resolved_turn, chat_result): - break - response_text = (chat_result.text_response or "").strip() if not response_text and chat_result.alert_proposals: response_text = render_alert_proposal(chat_result.alert_proposals[-1]) if not response_text: response_text = render_answer_text(chat_result) - if not response_text and not chat_result.tool_call_events: + transcript.append( + TranscriptEntry( + "assistant", " ".join(p for p in (response_text, summarize_visualizations(chat_result)) if p) + ) + ) + + if _check_output_present(resolved_turn, chat_result): + if incomplete: + records.append(ClarificationRecord(kind="turn_incomplete", agent_message=clip(response_text))) break - if clarification_turns >= max_clarification_turns: + + kind: ReplyRecordKind = "turn_incomplete" if incomplete else classify_reply(chat_result, response_text) + if ( + resolved_mode == "legacy" + and not incomplete + and not response_text + and not chat_result.tool_call_events + ): + records.append(ClarificationRecord(kind="no_action", agent_message="")) break + record = ClarificationRecord(kind=kind, agent_message=clip(response_text)) + if kind == "question": + # transcript ends [..., user: current_message, assistant: this reply]. + history, latest = render_transcript(transcript[:-2]), current_message + record.judge_input = judge_prompt(history, latest, response_text) + verdict = judge.judge(history, latest, response_text) + record.lost_context = verdict.lost_context + record.reasoning = verdict.reasoning + record.judge_error = verdict.error + # A second stall in the same turn ends it: one push is what a user gives. + second_stall = ( + resolved_mode == "context" + and kind in ("no_action", "turn_incomplete") + and any(r.kind in ("no_action", "turn_incomplete") for r in records) + ) + if clarification_turns >= max_clarification_turns or second_stall: + records.append(record) + break + + if resolved_mode == "legacy": + reply = _get_sim_user_response(response_text, resolved_turn, resolved_expected) + elif kind == "confirmation": + reply = CONFIRMATION_REPLY + elif kind in ("no_action", "turn_incomplete"): + reply = NUDGE_MESSAGE + elif record.lost_context is True and not any(r.lost_context is True for r in records): + # Once per turn, so a judge mistake cannot loop the user on restating. + reply = reply_restating(render_transcript(transcript[:-1]), response_text) + elif answers_left: + reply = answers_left.pop(0) + else: + reply = reply_from_facts(render_transcript(transcript[:-1]), response_text, turn.set_answers) + record.reply = clip(reply) + records.append(record) + clarification_turns += 1 total_clarification_turns += 1 - current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) + current_message = reply # `declared` is what THIS turn's final set_skills call asked for (None when it # made no call); `active_skills` is what is actually active during the turn. No @@ -477,9 +897,10 @@ def run_agentic_conversation( active_skills = set(declared) skill_routing = turn.expected_skill in active_skills output_present = _check_output_present(resolved_turn, final_result) if final_result else False - output_correct = ( - _check_output_correct(resolved_turn, final_result) if (final_result and output_present) else None - ) + output_correct: bool | None = None + mismatches: list[str] = [] + if final_result and output_present: + output_correct, mismatches = _check_output_correct(resolved_turn, final_result, all_tool_calls) # Capture metric output for $ref resolution in subsequent turns. if final_result and turn.expected_output_type == "metric": @@ -487,11 +908,6 @@ def run_agentic_conversation( if metric_data: turn_outputs[turn.turn_id] = metric_data - # Track every metric created this turn (any turn may create one) for cleanup. - for metric_id in _extract_created_metric_ids(all_tool_calls): - if metric_id not in created_metric_ids: - created_metric_ids.append(metric_id) - turn_results.append( TurnResult( turn_id=turn.turn_id, @@ -503,14 +919,16 @@ def run_agentic_conversation( active_skills=sorted(active_skills), clarification_turns_used=clarification_turns, output_correct=output_correct, + mismatches=mismatches, + clarifications=records, + depends_on=list(turn.depends_on), ) ) finally: if owns_conversation and conversation_id: client.delete_conversation(conversation_id) - for metric_id in created_metric_ids: - _delete_metric(sdk, workspace_id, metric_id) + created.delete(sdk, workspace_id) client.close() # Not derived from TurnResult.activated_skills: that field carries only each turn's FINAL @@ -518,6 +936,8 @@ def run_agentic_conversation( # been activated. See ever_declared_skills' declaration above. full_skill_coverage = set(fixture.expected_skills).issubset(ever_declared_skills) conversation_success = all(tr.skill_success for tr in turn_results) + context_success, kept_rate, before_break, lost, stalled = _context_summary(turn_results, fixture) + judged = any(c.kind == "question" for tr in turn_results for c in tr.clarifications) return ConversationResult( conversation_id=conversation_id, @@ -530,13 +950,28 @@ def run_agentic_conversation( response_id=response_id, tool_call_events=conversation_tool_call_events, reasoning_step_events=conversation_reasoning_step_events, + context_success=context_success, + context_kept_rate=kept_rate, + turns_before_first_break=before_break, + lost_context_clarifications=lost, + stalled_turns=stalled, + mode=resolved_mode, + judge_model=judge.model_name if judged else None, ) def _conversation_detail(result: ConversationResult) -> dict: return { + "mode": result.mode, "full_skill_coverage": result.full_skill_coverage, + "conversation_success": result.conversation_success, + "context_success": result.context_success, + "context_kept_rate": result.context_kept_rate, + "turns_before_first_break": result.turns_before_first_break, + "lost_context_clarifications": result.lost_context_clarifications, + "stalled_turns": result.stalled_turns, "total_clarification_turns": result.total_clarification_turns, + "judge_model": result.judge_model, "turns": [tr.detail() for tr in result.turn_results], **timeline_detail(result.tool_call_events, result.reasoning_step_events), } @@ -562,16 +997,20 @@ def evaluate_agentic_conversation( run_metadata_extra: dict | None = None, reasoning_effort: ReasoningEffort | None = None, submit_trace_link: SubmitTraceLink = run_trace_link_inline, + mode: ConversationMode | None = None, + clarification_judge: ClarificationJudge | None = None, ) -> AgenticEvalOutcome: """Run conversation evaluation, log to Langfuse, and raise on failure. - Returns the conversation's outcome (reasoning_steps, conversation_id, response_id) as - an AgenticEvalOutcome on success; on failure the same three values are attached to the - raised exception as + ``mode`` (default: ``GD_EVAL_CONVERSATION_MODE``, else ``legacy``) picks which verdict + raises; both are always scored. Returns the conversation's outcome (reasoning_steps, + conversation_id, response_id) as an AgenticEvalOutcome on success; on failure the same + three values are attached to the raised exception as ``.reasoning_steps``/``.conversation_id``/``.response_id`` (mirrors the `conversation_id`-on-exception idiom in `ChatClient.ask()`) so callers can retrieve them either way. """ + resolved_mode = resolve_conversation_mode(mode) langfuse, window_start = open_trace_window(langfuse) result = run_agentic_conversation( host=host, @@ -582,7 +1021,10 @@ def evaluate_agentic_conversation( initial_conversation_id=initial_conversation_id, reasoning_effort=reasoning_effort, agent_id=agent_id, + mode=resolved_mode, + clarification_judge=clarification_judge, ) + passed = result.context_success if resolved_mode == "context" else result.conversation_success if langfuse is not None and dataset_item_id: # Pinned on the calling thread: a deferred poll must not widen its query window. @@ -590,6 +1032,7 @@ def evaluate_agentic_conversation( # Resolved here, not inside the task: deferring it would make the queued task hold # the whole fixture until the drain. ds_name = dataset_name or fixture.dataset_name + failed_turns = {tr.turn_id: tr.failure_reasons() for tr in result.turn_results if not tr.context_success} def _write_scores(ctx: RunTraceContext) -> None: @@ -599,8 +1042,11 @@ def _write_scores(ctx: RunTraceContext) -> None: 0, conversation_id=result.conversation_id, output={ + "mode": result.mode, "conversation_success": result.conversation_success, + "context_success": result.context_success, "full_skill_coverage": result.full_skill_coverage, + "failed_turns": failed_turns, }, ) as tid: ctx.score( @@ -609,12 +1055,30 @@ def _write_scores(ctx: RunTraceContext) -> None: value=float(result.conversation_success), data_type="BOOLEAN", ) + ctx.score( + tid, + name="context_success", + value=float(result.context_success), + data_type="BOOLEAN", + ) ctx.score( tid, name="full_skill_coverage", value=float(result.full_skill_coverage), data_type="BOOLEAN", ) + if result.context_kept_rate is not None: + ctx.score(tid, name="context_kept_rate", value=result.context_kept_rate, data_type="NUMERIC") + ctx.score( + tid, name="turns_before_first_break", value=result.turns_before_first_break, data_type="NUMERIC" + ) + ctx.score( + tid, + name="lost_context_clarifications", + value=result.lost_context_clarifications, + data_type="NUMERIC", + ) + ctx.score(tid, name="stalled_turns", value=result.stalled_turns, data_type="NUMERIC") # One turn per fixture turn, plus every simulated-user round the agent triggered. # The clarification count alone hides how much of the conversation the fixture # asked for, so the comparison needs the total. @@ -631,6 +1095,8 @@ def _write_scores(ctx: RunTraceContext) -> None: value=result.total_clarification_turns, data_type="NUMERIC", ) + # Reports read gate_passed first; it must be the verdict this run raised on. + log_gate_scores(ctx, tid, gate=None, pass_at_k=passed, pass_power_k=passed) for tr in result.turn_results: ctx.score( tid, @@ -638,12 +1104,15 @@ def _write_scores(ctx: RunTraceContext) -> None: value=float(tr.skill_success), data_type="BOOLEAN", ) + strict_checks = { + "conversation_success": result.conversation_success, + "full_skill_coverage": result.full_skill_coverage, + } + if resolved_mode == "context": + strict_checks["context_success"] = result.context_success ctx.quality( tid, - strict_checks={ - "conversation_success": result.conversation_success, - "full_skill_coverage": result.full_skill_coverage, - }, + strict_checks=strict_checks, latency_sec=pt.latency if pt else None, cost_usd=pt.total_cost if pt else None, ) @@ -673,13 +1142,18 @@ def _write_scores(ctx: RunTraceContext) -> None: detail = _conversation_detail(result) - if not result.conversation_success: - failed_turns = [tr for tr in result.turn_results if not tr.skill_success] - exc = ConversationAssertionError( - f"Conversation assertion failed. " - f"full_skill_coverage={result.full_skill_coverage}. " - f"Failed turns: {[t.turn_id for t in failed_turns]}" - ) + if not passed: + if resolved_mode == "context": + failed = {tr.turn_id: tr.failure_reasons() for tr in result.turn_results if not tr.context_success} + message = f"Conversation assertion failed (context). Failed turns: {failed}" + else: + legacy_failed = [tr.turn_id for tr in result.turn_results if not tr.skill_success] + message = ( + f"Conversation assertion failed. " + f"full_skill_coverage={result.full_skill_coverage}. " + f"Failed turns: {legacy_failed}" + ) + exc = ConversationAssertionError(message) exc.reasoning_steps = result.reasoning_steps exc.conversation_id = result.conversation_id exc.response_id = result.response_id diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index f9fa0c746..2350b96f7 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -1,18 +1,35 @@ # (C) 2026 GoodData Corporation. All rights reserved. # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise +import json as _json +from datetime import date from unittest.mock import MagicMock, patch import pytest +from gooddata_eval.core.agentic._conversation_context import ( + CONFIRMATION_REPLY, + NUDGE_MESSAGE, + ClarificationVerdict, + _ClarificationLLM, + classify_reply, + judge_prompt, +) from gooddata_eval.core.agentic.conversation import ( ConversationAssertionError, ConversationFixture, TurnDefinition, TurnResult, + _canonical_maql, + _check_output_correct, _get_sim_user_response, _resolve_refs, evaluate_agentic_conversation, + resolve_conversation_mode, run_agentic_conversation, ) +from gooddata_eval.core.chat.render import render_answer_text +from gooddata_eval.core.chat.sse_client import ChatError as ChatError_ +from gooddata_eval.core.chat.sse_client import TurnIncompleteError +from gooddata_eval.core.evaluators._llm_judge import JudgeResponseError from gooddata_eval.core.models import ChatResult, ToolCallEvent @@ -126,7 +143,7 @@ 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 + assert set(_turn_result().detail()) == TurnResult._DETAIL_FIELDS | {"context_success", "failure_reasons"} def test_resolve_refs_no_refs(): @@ -946,8 +963,16 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): assert outcome.conversation_id == "conv-1" assert outcome.response_id == "resp-1" assert outcome.detail == { + "mode": "legacy", "full_skill_coverage": True, + "conversation_success": True, + "context_success": True, + "context_kept_rate": None, + "turns_before_first_break": 1, + "lost_context_clarifications": 0, + "stalled_turns": 0, "total_clarification_turns": 0, + "judge_model": None, "turns": [ { "turn_id": "t1", @@ -957,6 +982,12 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): "output_correct": None, "activated_skills": ["visualization"], "active_skills": ["visualization"], + "clarification_turns_used": 0, + "mismatches": [], + "clarifications": [], + "depends_on": [], + "context_success": True, + "failure_reasons": [], } ], "latency_breakdown": [], @@ -1011,8 +1042,16 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ assert exc_info.value.conversation_id == "conv-1" assert exc_info.value.response_id == "resp-2" assert exc_info.value.detail == { + "mode": "legacy", "full_skill_coverage": False, + "conversation_success": False, + "context_success": False, + "context_kept_rate": None, + "turns_before_first_break": 0, + "lost_context_clarifications": 0, + "stalled_turns": 1, "total_clarification_turns": 0, + "judge_model": None, "turns": [ { "turn_id": "t1", @@ -1022,6 +1061,25 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ "output_correct": None, "activated_skills": ["other_skill"], "active_skills": ["other_skill"], + "clarification_turns_used": 0, + "mismatches": [], + "clarifications": [ + { + "kind": "no_action", + "agent_message": "Here is something else", + "lost_context": None, + "reasoning": "", + "judge_error": None, + "judge_input": "", + "reply": "", + } + ], + "depends_on": [], + "context_success": False, + "failure_reasons": [ + "no output of the expected type", + "ended a reply without the expected output and without asking anything", + ], } ], "latency_breakdown": [], @@ -1131,3 +1189,512 @@ def _capture(_submit, _identity, **kwargs): assert scores["clarification_turns"] == 1 assert scores["turns"] == 2 # 1 fixture turn + 1 clarification round assert scores["steps"] == 5 + + +def _viz(metric="metric/gross_margin_percent", dim="label/dim_store.store_format", filters=None, sort=None): + query = {"fields": {"m": {"using": metric}, "d": {"using": dim}}, "filter_by": filters or {}} + if sort: + query["sort_by"] = sort + return {"type": "bar_chart", "query": query, "metrics": ["m"], "view_by": ["d"]} + + +_EXPRESS = { + "f": {"type": "attribute_filter", "using": "label/dim_store.store_format", "state": {"include": ["Express"]}} +} + + +def _tool(name, args=None, result=None): + return { + "functionName": name, + "functionArguments": _json.dumps(args or {}), + "result": _json.dumps(result) if result is not None else None, + } + + +def _result(text="Done", viz=None, tools=None, **extra): + payload = {"textResponse": text, "toolCallEvents": tools or []} + if viz is not None: + payload["createdVisualizations"] = {"objects": [viz]} + payload.update(extra) + return ChatResult.model_validate(payload) + + +def _viz_turn(turn_id="t1", expected=None, **kw): + return TurnDefinition( + turn_id=turn_id, + message=kw.pop("message", "chart it"), + expected_skill="visualization", + expected_output={"visualization": expected} if expected is not None else None, + **kw, + ) + + +def test_output_correct_reads_the_wrapped_visualization_and_passes_an_identical_chart(): + ok, diff = _check_output_correct(_viz_turn(expected=_viz(filters=_EXPRESS)), _result(viz=_viz(filters=_EXPRESS))) + assert (ok, diff) == (True, []) + + +def test_output_correct_fails_a_chart_that_dropped_an_established_filter(): + ok, diff = _check_output_correct(_viz_turn(expected=_viz(filters=_EXPRESS)), _result(viz=_viz())) + assert ok is False + assert any(d.startswith("attribute filters") for d in diff) + + +def test_output_correct_fails_a_filter_nobody_asked_for(): + ok, diff = _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(filters=_EXPRESS))) + assert ok is False + assert any(d.startswith("attribute filters") for d in diff) + + +def test_output_correct_fails_a_different_metric(): + ok, diff = _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(metric="metric/units_sold"))) + assert ok is False + assert diff[0].startswith("metrics") + + +def test_output_correct_accepts_any_listed_alternative(): + turn = _viz_turn(expected=_viz(filters=_EXPRESS), expected_output_alternatives=[{"visualization": _viz()}]) + assert _check_output_correct(turn, _result(viz=_viz()))[0] is True + + +def test_output_correct_checks_sort_only_when_expected_states_it(): + sort = [{"type": "metric_sort", "direction": "DESC", "metrics": ["m"]}] + assert _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(sort=sort)))[0] is True + ok, diff = _check_output_correct(_viz_turn(expected=_viz(sort=sort)), _result(viz=_viz())) + assert ok is False + assert diff[0].startswith("sort") + + +def test_output_correct_sort_without_a_direction_matches_either_direction(): + by_units = [{"type": "metric_sort", "metrics": ["m"]}] + asc = [{"type": "metric_sort", "direction": "ASC", "metrics": ["m"]}] + assert _check_output_correct(_viz_turn(expected=_viz(sort=by_units)), _result(viz=_viz(sort=asc)))[0] is True + desc = [{"type": "metric_sort", "direction": "DESC", "metrics": ["m"]}] + assert _check_output_correct(_viz_turn(expected=_viz(sort=desc)), _result(viz=_viz(sort=asc)))[0] is False + + +def test_output_correct_is_unknown_when_nothing_is_expected(): + assert _check_output_correct(_viz_turn(), _result(viz=_viz())) == (None, []) + + +def test_output_correct_compares_tool_arguments_across_the_turn(): + turn = TurnDefinition( + turn_id="a", + message="alert me", + expected_skill="alert", + expected_output_type="tool_call", + expected_tool_name="create_metric_alert", + expected_tool_args={"metric_id": "units_sold", "threshold": 25}, + ) + call = ToolCallEvent.model_validate(_tool("create_metric_alert", {"metric_id": "units_sold", "threshold": 25})) + wrong = ToolCallEvent.model_validate(_tool("create_metric_alert", {"metric_id": "units_sold", "threshold": 250})) + assert _check_output_correct(turn, _result(), [call])[0] is True + assert _check_output_correct(turn, _result(), [wrong])[0] is False + + +def test_output_correct_compares_maql_for_a_metric_turn(): + turn = TurnDefinition( + turn_id="m", + message="make it", + expected_skill="metric", + expected_output_type="metric", + expected_output={"maql": "SELECT {metric/a} / {metric/b}"}, + ) + good = ToolCallEvent.model_validate( + _tool("create_metric", result={"data": {"metric_id": "x", "maql": "SELECT {metric/a}/{metric/b}"}}) + ) + bad = ToolCallEvent.model_validate( + _tool("create_metric", result={"data": {"metric_id": "x", "maql": "SELECT {metric/a}"}}) + ) + assert _check_output_correct(turn, _result(), [good])[0] is True + assert _check_output_correct(turn, _result(), [bad])[0] is False + + +@pytest.mark.parametrize( + ("result", "kind"), + [ + (_result(text="Formula: SELECT 1. Should I create this metric?"), "confirmation"), + (_result(text=None, alertProposals=[{"cta": "Create?"}]), "confirmation"), + (_result(text="Which revenue metric do you mean?"), "question"), + (_result(text="Please select one of the five options by number, name, or ID."), "question"), + (_result(text="Reply with the number of the dashboard to use."), "question"), + (_result(text="Let me check what that maps to."), "no_action"), + (_result(text=None), "no_action"), + (_result(text="Pick one", unhandledParts=[{"type": "clarifyingQuestions"}]), "question"), + ], +) +def test_classify_reply(result, kind): + assert classify_reply(result, result.text_response or render_answer_text(result)) == kind + + +def test_resolve_conversation_mode(monkeypatch): + monkeypatch.delenv("GD_EVAL_CONVERSATION_MODE", raising=False) + assert resolve_conversation_mode() == "legacy" + monkeypatch.setenv("GD_EVAL_CONVERSATION_MODE", "context") + assert resolve_conversation_mode() == "context" + assert resolve_conversation_mode("legacy") == "legacy" + with pytest.raises(ValueError, match="legacy"): + resolve_conversation_mode("strict") + + +class _FakeJudge: + """Grades every question with a fixed verdict and records what it was shown.""" + + model_name = "fake-judge" + + def __init__(self, lost_context): + self.lost_context = lost_context + self.calls = [] + + def judge(self, history, latest, question): + self.calls.append((history, latest, question)) + return ClarificationVerdict(lost_context=self.lost_context, reasoning="because") + + +_SKILLS = _tool("set_skills", {"skill_names": ["visualization"]}) + + +def _run(replies, turns, mode="context", judge=None, **kw): + client = MagicMock() + client.create_conversation.return_value = "conv-1" + client.send_message.side_effect = replies + fixture = ConversationFixture(id="c", expected_skills=["visualization"], turns=turns) + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + ): + result = run_agentic_conversation( + host="h", token="t", workspace_id="ws", fixture=fixture, mode=mode, clarification_judge=judge, **kw + ) + return result, client + + +def test_context_mode_fails_a_turn_that_asks_for_what_the_conversation_established(): + turns = [ + _viz_turn("t1", expected=_viz()), + _viz_turn("t2", expected=_viz(filters=_EXPRESS), message="its margin?", depends_on=["t1"]), + ] + judge = _FakeJudge(lost_context=True) + with patch( + "gooddata_eval.core.agentic.conversation.reply_restating", return_value="As I said earlier, Express." + ) as restate: + result, client = _run( + [ + _result(viz=_viz(), tools=[_SKILLS]), + _result(text="Which store format do you mean?"), + _result(viz=_viz(filters=_EXPRESS)), + ], + turns, + judge=judge, + ) + t2 = result.turn_results[1] + assert t2.skill_success is True # the legacy verdict still passes it + assert t2.lost_context is True + assert t2.context_success is False + assert "asked for information the conversation already established" in t2.failure_reasons() + assert result.conversation_success is True + assert result.context_success is False + assert result.context_kept_rate == 0.0 + assert result.turns_before_first_break == 1 + assert result.lost_context_clarifications == 1 + restate.assert_called_once() + assert client.send_message.call_args_list[2].args[1] == "As I said earlier, Express." + history, latest, question = judge.calls[0] + assert "chart it" in history + assert latest == "its margin?" + assert "its margin?" in t2.clarifications[0].judge_input + assert question == "Which store format do you mean?" + + +def test_context_mode_sends_the_set_answer_to_a_legitimate_question(): + turns = [_viz_turn("t1", expected=_viz(), message="revenue by format", set_answers=["Gross Margin Percent."])] + result, client = _run( + [_result(text="Which metric do you mean?"), _result(viz=_viz(), tools=[_SKILLS])], + turns, + judge=_FakeJudge(lost_context=False), + ) + assert client.send_message.call_args_list[1].args[1] == "Gross Margin Percent." + assert result.turn_results[0].context_success is True + assert result.turn_results[0].clarifications[0].reply == "Gross Margin Percent." + + +def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_against_the_turn(): + turns = [_viz_turn("t1", expected=_viz())] + judge = _FakeJudge(lost_context=True) + result, client = _run( + [_result(text="Should I create this chart?"), _result(viz=_viz(), tools=[_SKILLS])], turns, judge=judge + ) + assert client.send_message.call_args_list[1].args[1] == CONFIRMATION_REPLY + assert judge.calls == [] + assert result.turn_results[0].context_success is True + + +def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges(): + turns = [_viz_turn("t1", expected=_viz())] + incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations") + result, client = _run([incomplete, _result(viz=_viz(), tools=[_SKILLS])], turns, judge=_FakeJudge(False)) + turn = result.turn_results[0] + assert client.send_message.call_args_list[1].args[1] == NUDGE_MESSAGE + assert turn.output_present is True + assert turn.stalled is True + assert turn.context_success is False + assert result.stalled_turns == 1 + + +def test_context_mode_never_primes_the_simulated_user_with_the_expected_output(): + turns = [_viz_turn("t1", expected=_viz())] + with patch("gooddata_eval.core.agentic.conversation._get_sim_user_response") as legacy_sim: + _run([_result(text="Which metric?"), _result(viz=_viz(), tools=[_SKILLS])], turns, judge=_FakeJudge(False)) + legacy_sim.assert_not_called() + + +def test_legacy_mode_keeps_the_primed_simulated_user_and_the_legacy_verdict(): + turns = [_viz_turn("t1", expected=_viz(filters=_EXPRESS))] + with patch("gooddata_eval.core.agentic.conversation._get_sim_user_response", return_value="Express") as sim: + result, _ = _run( + [_result(text="Which format?"), _result(viz=_viz(), tools=[_SKILLS])], + turns, + mode="legacy", + judge=_FakeJudge(False), + ) + sim.assert_called_once() + assert result.conversation_success is True # legacy ignores the dropped filter + assert result.context_success is False # ...which context mode reports + assert result.turn_results[0].output_correct is False + + +def test_context_kept_rate_is_over_labelled_turns_only(): + turns = [ + _viz_turn("t1", expected=_viz()), + _viz_turn("t2", expected=_viz(), depends_on=["t1"]), + _viz_turn("t3", expected=_viz(filters=_EXPRESS)), + ] + good = _result(viz=_viz(), tools=[_SKILLS]) + result, _ = _run([good, good, good], turns, judge=_FakeJudge(False)) + # t3 fails but is not labelled context-dependent, so the rate is t2 alone. + assert result.context_kept_rate == 1.0 + assert result.turns_before_first_break == 2 + assert result.context_success is False + + +def test_fresh_conversation_per_turn_gives_every_turn_an_empty_history(): + turns = [_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz())] + good = _result(viz=_viz(), tools=[_SKILLS]) + _, client = _run([good, good], turns, judge=_FakeJudge(False), fresh_conversation_per_turn=True) + assert client.create_conversation.call_count == 2 + assert client.delete_conversation.call_count == 2 + + +def test_cleanup_deletes_created_metrics_and_alerts_but_keeps_a_metric_updated_in_place(): + turns = [_viz_turn("t1", expected=_viz())] + tools = [ + _SKILLS, + _tool("create_metric", result={"data": {"metric_id": "new_one", "maql": "SELECT 1", "created_new": True}}), + _tool("create_metric", result={"data": {"metric_id": "old_one", "maql": "SELECT 2", "created_new": False}}), + _tool("create_metric_alert", result={"id": "alert-1"}), + ] + with ( + patch("gooddata_eval.core.agentic.conversation._delete_metric") as del_metric, + patch("gooddata_eval.core.agentic.conversation._delete_alert") as del_alert, + ): + _run([_result(viz=_viz(), tools=tools)], turns, judge=_FakeJudge(False)) + assert [c.args[2] for c in del_metric.call_args_list] == ["new_one"] + assert [c.args[2] for c in del_alert.call_args_list] == ["alert-1"] + + +def test_cleanup_still_runs_for_a_metric_created_before_the_stream_died(): + turns = [_viz_turn("t1", expected=_viz())] + partial = _result(tools=[_tool("create_metric", result={"data": {"metric_id": "orphan", "maql": "SELECT 1"}})]) + with ( + patch("gooddata_eval.core.agentic.conversation._delete_metric") as del_metric, + pytest.raises(ChatError_), + ): + _run([ChatError_("stream died", partial_result=partial)], turns, judge=_FakeJudge(False)) + assert [c.args[2] for c in del_metric.call_args_list] == ["orphan"] + + +def test_context_mode_pushes_a_stalled_turn_once_then_ends_it(): + turns = [_viz_turn("t1", expected=_viz())] + stall = _result(text="Let me look into that.") + result, client = _run([stall, stall, stall], turns, judge=_FakeJudge(False)) + assert client.send_message.call_count == 2 + assert [c.kind for c in result.turn_results[0].clarifications] == ["no_action", "no_action"] + assert result.turn_results[0].context_success is False + + +@pytest.mark.parametrize(("mode", "gate"), [("legacy", True), ("context", False)]) +def test_scores_carry_both_verdicts_and_gate_on_the_mode(mode, gate): + """A turn that shows the chart but drops an established filter: legacy passes it, context does not.""" + client = MagicMock() + client.create_conversation.return_value = "conv-1" + client.send_message.side_effect = [_result(viz=_viz(), tools=[_SKILLS])] + fixture = ConversationFixture( + id="c", expected_skills=["visualization"], turns=[_viz_turn("t1", expected=_viz(filters=_EXPRESS))] + ) + captured = {} + + def _capture(_submit, _identity, **kwargs): + captured["write_scores"] = kwargs["write_scores"] + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + patch("gooddata_eval.core.agentic.conversation.submit_trace_scoring", _capture), + ): + try: + evaluate_agentic_conversation( + host="h", + token="t", + workspace_id="ws", + fixture=fixture, + langfuse=MagicMock(), + dataset_item_id="item-1", + mode=mode, + clarification_judge=_FakeJudge(False), + ) + raised = False + except ConversationAssertionError: + raised = True + assert raised is not gate + + ctx = MagicMock() + captured["write_scores"](ctx) + scores = {c.kwargs["name"]: c.kwargs["value"] for c in ctx.score.call_args_list} + assert scores["conversation_success"] == 1.0 + assert scores["context_success"] == 0.0 + assert scores["gate_passed"] is gate + assert scores["turns_before_first_break"] == 0 + + +def test_output_correct_treats_an_end_date_after_today_as_today(): + """ "2025 and 2026 so far": the year end and today select the same data.""" + so_far = { + "f": {"type": "date_filter", "using": "dataset/transaction_date", "from": "2025-01-01", "to": "2026-12-31"} + } + today = { + "f": {"type": "date_filter", "using": "dataset/transaction_date", "from": "2025-01-01", "to": "2026-09-30"} + } + earlier = { + "f": {"type": "date_filter", "using": "dataset/transaction_date", "from": "2025-01-01", "to": "2026-06-30"} + } + with patch("gooddata_eval.core.agentic.conversation.date") as fake_date: + fake_date.today.return_value = date(2026, 9, 30) + assert _check_output_correct(_viz_turn(expected=_viz(filters=so_far)), _result(viz=_viz(filters=today)))[0] + assert not _check_output_correct(_viz_turn(expected=_viz(filters=so_far)), _result(viz=_viz(filters=earlier)))[ + 0 + ] + + +def _llm_returning(body): + llm = _ClarificationLLM.__new__(_ClarificationLLM) + llm.model = "fake" + llm._system_prompt = "" + choice = MagicMock() + choice.message.content = body + llm._create_completion = lambda messages: MagicMock(choices=[choice]) + return llm + + +def test_clarification_llm_reads_the_named_boolean(): + assert _llm_returning('{"already_in_conversation": true, "reasoning": "t2 said so"}').already_in_conversation( + "p" + ) == (True, "t2 said so") + assert ( + _llm_returning('{"already_in_conversation": false, "reasoning": "no"}').already_in_conversation("p")[0] is False + ) + + +@pytest.mark.parametrize("body", ['{"score": 0}', '{"already_in_conversation": "yes"}', "not json", ""]) +def test_clarification_llm_refuses_anything_but_a_boolean_verdict(body): + with pytest.raises(JudgeResponseError): + _llm_returning(body).already_in_conversation("p") + + +def test_judge_prompt_marks_an_empty_history(): + assert "CONVERSATION BEFORE:\n(empty)" in judge_prompt("", "Sort it", "By what?") + + +def test_context_mode_restates_at_most_once_per_turn(): + turns = [_viz_turn("t1", expected=_viz(), set_answers=["Gross Margin Percent."])] + with patch( + "gooddata_eval.core.agentic.conversation.reply_restating", return_value="As I said earlier, X." + ) as restate: + _, client = _run( + [_result(text="Which one?"), _result(text="Which one, again?"), _result(viz=_viz(), tools=[_SKILLS])], + turns, + judge=_FakeJudge(lost_context=True), + ) + restate.assert_called_once() + assert client.send_message.call_args_list[2].args[1] == "Gross Margin Percent." + + +def test_output_correct_treats_a_single_value_in_as_equals(): + turn = TurnDefinition( + turn_id="m", + message="make it", + expected_skill="metric", + expected_output_type="metric", + expected_output={"maql": 'SELECT {metric/a} WHERE {label/b} = "Digital"'}, + ) + same = ToolCallEvent.model_validate( + _tool( + "create_metric", + result={"data": {"metric_id": "x", "maql": 'SELECT {metric/a} WHERE {label/b} IN ("Digital")'}}, + ) + ) + two = ToolCallEvent.model_validate( + _tool( + "create_metric", + result={"data": {"metric_id": "x", "maql": 'SELECT {metric/a} WHERE {label/b} IN ("Digital", "Online")'}}, + ) + ) + assert _check_output_correct(turn, _result(), [same])[0] is True + assert _check_output_correct(turn, _result(), [two])[0] is False + + +def test_an_iteration_limit_after_the_chart_was_made_keeps_the_chart(): + turns = [_viz_turn("t1", expected=_viz())] + partial = _result(viz=_viz(), tools=[_SKILLS]) + incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations", partial_result=partial) + result, client = _run([incomplete], turns, judge=_FakeJudge(False)) + turn = result.turn_results[0] + assert client.send_message.call_count == 1 + assert turn.output_present is True and turn.output_correct is True + assert turn.stalled is True + + +def test_a_fresh_conversation_starts_with_no_active_skills(): + turns = [_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz())] + result, _ = _run( + [_result(viz=_viz(), tools=[_SKILLS]), _result(viz=_viz())], + turns, + judge=_FakeJudge(False), + fresh_conversation_per_turn=True, + ) + assert result.turn_results[1].skill_routing is False + + +def test_a_question_on_the_last_allowed_round_is_still_judged(): + turns = [_viz_turn("t1", expected=_viz())] + judge = _FakeJudge(lost_context=True) + result, _ = _run([_result(text="Which chart?")], turns, judge=judge, max_clarification_turns=0) + assert result.turn_results[0].clarifications[0].lost_context is True + assert result.lost_context_clarifications == 1 + + +def test_not_in_is_not_rewritten_as_equals(): + assert "not in" in _canonical_maql('SELECT {metric/a} WHERE {label/b} NOT IN ("x")') + assert _canonical_maql('SELECT {metric/a} WHERE {label/b} IN ("x")') == _canonical_maql( + 'SELECT {metric/a} WHERE {label/b} = "x"' + ) + + +def test_the_flat_expected_shape_is_still_checked(): + turn = TurnDefinition( + turn_id="t", + message="m", + expected_skill="visualization", + expected_output={"metrics": ["metric/gross_margin_percent"], "dimensions": ["label/dim_store.store_format"]}, + ) + assert _check_output_correct(turn, _result(viz=_viz()))[0] is True + assert _check_output_correct(turn, _result(viz=_viz(metric="metric/units_sold")))[0] is False From c66ac11ad56b5b68c6a9bdf5104fd31d264e76e9 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 1 Oct 2026 12:20:54 +0700 Subject: [PATCH 4/5] test(gooddata-eval): cover the clarification judge and simulated user Unit tests for the paths of the context mode that had none: the judge's cache, an unreadable verdict, a provider fault announced once, the empty-body retry; the simulated user answering from facts, restating, and falling back without a key, on a provider error or an empty body; and the content check's edges (no chart or metric to compare, unreadable expected output or tool result, dimension and type mismatches). Drops a guard in _sort_signature the model already makes unreachable. jira: QA-29448 risk: nonprod --- .../core/agentic/conversation.py | 2 - .../tests/test_agentic_conversation.py | 226 ++++++++++++++++++ 2 files changed, 226 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 5385c644b..f501c5a9c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -361,8 +361,6 @@ def _sort_signature(viz: CreatedVisualization) -> set[tuple[str, str, str]]: """(sort type, sorted-by URI, direction) for each sort entry.""" out: set[tuple[str, str, str]] = set() for entry in viz.query.sort_by: - if not isinstance(entry, dict): - continue stype = str(entry.get("type") or "") direction = str(entry.get("direction") or "").upper() aliases = entry.get("metrics") if stype == "metric_sort" else [entry.get("by")] diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 2350b96f7..da4e79eb9 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -5,13 +5,18 @@ from unittest.mock import MagicMock, patch import pytest +from gooddata_eval.core.agentic import _conversation_context as ctx_mod from gooddata_eval.core.agentic._conversation_context import ( CONFIRMATION_REPLY, + NO_ANSWER_REPLY, NUDGE_MESSAGE, + ClarificationJudge, ClarificationVerdict, _ClarificationLLM, classify_reply, judge_prompt, + reply_from_facts, + reply_restating, ) from gooddata_eval.core.agentic.conversation import ( ConversationAssertionError, @@ -20,7 +25,10 @@ TurnResult, _canonical_maql, _check_output_correct, + _created_alert_ids, + _expected_viz, _get_sim_user_response, + _metric_creations, _resolve_refs, evaluate_agentic_conversation, resolve_conversation_mode, @@ -1698,3 +1706,221 @@ def test_the_flat_expected_shape_is_still_checked(): ) assert _check_output_correct(turn, _result(viz=_viz()))[0] is True assert _check_output_correct(turn, _result(viz=_viz(metric="metric/units_sold")))[0] is False + + +# --- clarification judge ------------------------------------------------------------- + + +def _judge_with(llm): + judge = ClarificationJudge(model="fake") + judge._llm = llm + return judge + + +def test_judge_reads_the_verdict_and_caches_it_per_prompt(): + llm = MagicMock() + llm.already_in_conversation.return_value = (True, "t1 named it") + judge = _judge_with(llm) + first = judge.judge("USER: chart it", "Add Units Sold", "Which chart?") + second = judge.judge("USER: chart it", "Add Units Sold", "Which chart?") + assert first == second == ClarificationVerdict(lost_context=True, reasoning="t1 named it") + llm.already_in_conversation.assert_called_once() + judge.judge("", "Add Units Sold", "Which chart?") + assert llm.already_in_conversation.call_count == 2 + + +def test_an_unreadable_verdict_leaves_the_clarification_unjudged(): + llm = MagicMock() + llm.already_in_conversation.side_effect = JudgeResponseError("no boolean") + verdict = _judge_with(llm).judge("", "x", "y") + assert verdict.lost_context is None + assert "no boolean" in verdict.error + + +def test_a_provider_fault_leaves_clarifications_unjudged_and_is_announced_once(capsys): + llm = MagicMock() + llm.already_in_conversation.side_effect = RuntimeError("401 bad key") + judge = _judge_with(llm) + assert judge.judge("", "a", "q1").lost_context is None + assert judge.judge("", "b", "q2").error == "401 bad key" + assert capsys.readouterr().out.count("left unjudged") == 1 + + +def test_the_judge_needs_no_api_key_until_it_is_asked(monkeypatch): + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + judge = ClarificationJudge() + assert judge.model_name + assert judge.judge("", "a", "q").error is not None + + +def test_the_clarification_llm_uses_its_own_prompt(monkeypatch): + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + llm = _ClarificationLLM(model="gpt-4o") + assert "already_in_conversation" in llm._system_prompt + assert llm.model == "gpt-4o" + + +def test_the_clarification_llm_retries_an_empty_body_once(): + llm = _llm_returning("") + calls = [] + choice = MagicMock() + choice.message.content = '{"already_in_conversation": false, "reasoning": "r"}' + bodies = iter([MagicMock(choices=[]), MagicMock(choices=[choice])]) + llm._create_completion = lambda messages: calls.append(1) or next(bodies) + assert llm.already_in_conversation("p") == (False, "r") + assert len(calls) == 2 + + +# --- simulated user (context mode) ------------------------------------------------------- + + +def _openai_returning(content): + response = MagicMock() + response.choices = [MagicMock()] + response.choices[0].message.content = content + client = MagicMock() + client.chat.completions.create.return_value = response + return MagicMock(return_value=client) + + +def test_reply_from_facts_without_facts_needs_no_llm(): + with patch("openai.OpenAI") as openai_cls: + assert reply_from_facts("USER: x", "Which?", []) == NO_ANSWER_REPLY + openai_cls.assert_not_called() + + +def test_reply_from_facts_answers_from_the_facts(monkeypatch): + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + with patch("openai.OpenAI", _openai_returning(" Sales Order Revenue. ")) as openai_cls: + assert reply_from_facts("USER: revenue by month", "Which revenue?", ["Sales Order Revenue."]) == ( + "Sales Order Revenue." + ) + prompt = openai_cls.return_value.chat.completions.create.call_args.kwargs["messages"][1]["content"] + assert "Sales Order Revenue." in prompt and "Which revenue?" in prompt + + +@pytest.mark.parametrize("failure", ["no_key", "provider_error", "empty_body"]) +def test_simulated_user_falls_back_when_the_llm_gives_nothing(monkeypatch, failure): + if failure == "no_key": + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + openai_cls = _openai_returning("unused") + else: + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + openai_cls = _openai_returning(None) + if failure == "provider_error": + openai_cls.return_value.chat.completions.create.side_effect = RuntimeError("503") + with patch("openai.OpenAI", openai_cls): + assert reply_from_facts("h", "q", ["fact"]) == NO_ANSWER_REPLY + assert reply_restating("h", "q").startswith("As I said earlier") + + +def test_reply_restating_returns_the_llm_restatement(monkeypatch): + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + with patch("openai.OpenAI", _openai_returning("As I said earlier, Express.")): + assert reply_restating("USER: weakest? ASSISTANT: Express", "Which format?") == "As I said earlier, Express." + + +def test_simulated_user_reports_a_missing_openai_package(monkeypatch): + monkeypatch.setitem(__import__("sys").modules, "openai", None) + assert ctx_mod._chat("s", "u") is None + + +# --- turn verdict and content-check edges ------------------------------------------------- + + +def test_failure_reasons_name_an_unrun_turn_and_an_inactive_skill(): + unrun = _turn_result().model_copy(update={"no_error": False}) + assert unrun.failure_reasons()[0] == "turn could not run" + wrong_skill = _turn_result().model_copy(update={"skill_routing": False}) + assert wrong_skill.failure_reasons() == ["skill 'visualization' not active"] + + +def test_expected_viz_is_none_for_a_shape_it_cannot_read(): + assert _expected_viz({"visualization": "not a chart"}) is None + assert _expected_viz({"visualization": {"query": {"fields": {"m": 1}}}}) is None + assert _expected_viz({"visualization": {"query": {}}}) is None + + +def test_mismatches_name_the_dimension_and_the_type(): + expected = {**_viz(), "type": "line_chart"} + ok, diff = _check_output_correct(_viz_turn(expected=expected), _result(viz=_viz(dim="label/dim_brand.brand_name"))) + assert ok is False + assert any(d.startswith("dimensions") for d in diff) + assert any(d.startswith("type") for d in diff) + + +def test_output_correct_without_anything_to_compare_against(): + assert _check_output_correct(_viz_turn(expected=_viz()), _result()) == (False, ["no visualization to compare"]) + metric_turn = TurnDefinition( + turn_id="m", + message="make it", + expected_skill="metric", + expected_output_type="metric", + expected_output={"maql": "SELECT 1"}, + ) + assert _check_output_correct(metric_turn, _result(), []) == (False, ["no created metric to compare"]) + no_maql = metric_turn.model_copy(update={"expected_output": {"title": "x"}}) + assert _check_output_correct(no_maql, _result(), []) == (None, []) + + +def test_cleanup_ignores_tool_results_it_cannot_read(): + unreadable = [ + ToolCallEvent.model_validate(_tool("create_metric", result=["not", "a", "dict"])), + ToolCallEvent.model_validate(_tool("create_metric", result={"data": {"isError": True}})), + ToolCallEvent.model_validate(_tool("create_metric_alert", result=["nope"])), + ToolCallEvent.model_validate(_tool("create_metric_alert", result={"data": {"id": "a-nested"}})), + ] + assert _metric_creations(unreadable) == [] + assert _created_alert_ids(unreadable) == ["a-nested"] + + +def test_fresh_conversation_per_turn_refuses_a_conversation_it_did_not_create(): + fixture = ConversationFixture(id="c", expected_skills=["visualization"], turns=[_viz_turn("t1")]) + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient"), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + pytest.raises(ValueError, match="creates itself"), + ): + run_agentic_conversation( + host="h", + token="t", + workspace_id="ws", + fixture=fixture, + initial_conversation_id="given", + fresh_conversation_per_turn=True, + ) + + +def test_context_kept_rate_is_scored_when_there_are_dependent_turns(): + client = MagicMock() + client.create_conversation.return_value = "conv-1" + client.send_message.side_effect = [_result(viz=_viz(), tools=[_SKILLS]), _result(viz=_viz())] + fixture = ConversationFixture( + id="c", + expected_skills=["visualization"], + turns=[_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz(), depends_on=["t1"])], + ) + captured = {} + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + patch( + "gooddata_eval.core.agentic.conversation.submit_trace_scoring", + lambda _s, _i, **kw: captured.update(write=kw["write_scores"]), + ), + ): + evaluate_agentic_conversation( + host="h", + token="t", + workspace_id="ws", + fixture=fixture, + langfuse=MagicMock(), + dataset_item_id="i", + mode="context", + clarification_judge=_FakeJudge(False), + ) + ctx = MagicMock() + captured["write"](ctx) + scores = {c.kwargs["name"]: c.kwargs["value"] for c in ctx.score.call_args_list} + assert scores["context_kept_rate"] == 1.0 + assert scores["turns_before_first_break"] == 2 From ee39a7517d46f10852790e338eafb36c99d93031 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 1 Oct 2026 12:37:47 +0700 Subject: [PATCH 5/5] test(gooddata-eval): annotate the conversation tests, drop the openai patches Type annotations on the helpers and tests added for the context mode, as AGENTS.md asks of every function. The simulated-user tests patched `openai.OpenAI`, which fails before the code under test runs wherever the optional openai package is absent (a root `uv run pytest`); they now put a stand-in openai module in sys.modules, so they pass with and without the llm-judge extra. jira: QA-29448 risk: nonprod --- .../tests/test_agentic_conversation.py | 185 ++++++++++-------- .../tests/test_agentic_metric_skill.py | 4 +- .../gooddata-eval/tests/test_sse_client.py | 4 +- 3 files changed, 107 insertions(+), 86 deletions(-) diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index da4e79eb9..e720808a2 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -1,6 +1,8 @@ # (C) 2026 GoodData Corporation. All rights reserved. # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise import json as _json +import sys +import types from datetime import date from unittest.mock import MagicMock, patch @@ -21,6 +23,7 @@ from gooddata_eval.core.agentic.conversation import ( ConversationAssertionError, ConversationFixture, + ConversationResult, TurnDefinition, TurnResult, _canonical_maql, @@ -1199,8 +1202,13 @@ def _capture(_submit, _identity, **kwargs): assert scores["steps"] == 5 -def _viz(metric="metric/gross_margin_percent", dim="label/dim_store.store_format", filters=None, sort=None): - query = {"fields": {"m": {"using": metric}, "d": {"using": dim}}, "filter_by": filters or {}} +def _viz( + metric: str = "metric/gross_margin_percent", + dim: str = "label/dim_store.store_format", + filters: dict | None = None, + sort: list[dict] | None = None, +) -> dict: + query: dict = {"fields": {"m": {"using": metric}, "d": {"using": dim}}, "filter_by": filters or {}} if sort: query["sort_by"] = sort return {"type": "bar_chart", "query": query, "metrics": ["m"], "view_by": ["d"]} @@ -1211,7 +1219,7 @@ def _viz(metric="metric/gross_margin_percent", dim="label/dim_store.store_format } -def _tool(name, args=None, result=None): +def _tool(name: str, args: dict | None = None, result: object = None) -> dict: return { "functionName": name, "functionArguments": _json.dumps(args or {}), @@ -1219,15 +1227,17 @@ def _tool(name, args=None, result=None): } -def _result(text="Done", viz=None, tools=None, **extra): - payload = {"textResponse": text, "toolCallEvents": tools or []} +def _result( + text: str | None = "Done", viz: dict | None = None, tools: list[dict] | None = None, **extra: object +) -> ChatResult: + payload: dict = {"textResponse": text, "toolCallEvents": tools or []} if viz is not None: payload["createdVisualizations"] = {"objects": [viz]} payload.update(extra) return ChatResult.model_validate(payload) -def _viz_turn(turn_id="t1", expected=None, **kw): +def _viz_turn(turn_id: str = "t1", expected: dict | None = None, **kw: object) -> TurnDefinition: return TurnDefinition( turn_id=turn_id, message=kw.pop("message", "chart it"), @@ -1237,35 +1247,35 @@ def _viz_turn(turn_id="t1", expected=None, **kw): ) -def test_output_correct_reads_the_wrapped_visualization_and_passes_an_identical_chart(): +def test_output_correct_reads_the_wrapped_visualization_and_passes_an_identical_chart() -> None: ok, diff = _check_output_correct(_viz_turn(expected=_viz(filters=_EXPRESS)), _result(viz=_viz(filters=_EXPRESS))) assert (ok, diff) == (True, []) -def test_output_correct_fails_a_chart_that_dropped_an_established_filter(): +def test_output_correct_fails_a_chart_that_dropped_an_established_filter() -> None: ok, diff = _check_output_correct(_viz_turn(expected=_viz(filters=_EXPRESS)), _result(viz=_viz())) assert ok is False assert any(d.startswith("attribute filters") for d in diff) -def test_output_correct_fails_a_filter_nobody_asked_for(): +def test_output_correct_fails_a_filter_nobody_asked_for() -> None: ok, diff = _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(filters=_EXPRESS))) assert ok is False assert any(d.startswith("attribute filters") for d in diff) -def test_output_correct_fails_a_different_metric(): +def test_output_correct_fails_a_different_metric() -> None: ok, diff = _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(metric="metric/units_sold"))) assert ok is False assert diff[0].startswith("metrics") -def test_output_correct_accepts_any_listed_alternative(): +def test_output_correct_accepts_any_listed_alternative() -> None: turn = _viz_turn(expected=_viz(filters=_EXPRESS), expected_output_alternatives=[{"visualization": _viz()}]) assert _check_output_correct(turn, _result(viz=_viz()))[0] is True -def test_output_correct_checks_sort_only_when_expected_states_it(): +def test_output_correct_checks_sort_only_when_expected_states_it() -> None: sort = [{"type": "metric_sort", "direction": "DESC", "metrics": ["m"]}] assert _check_output_correct(_viz_turn(expected=_viz()), _result(viz=_viz(sort=sort)))[0] is True ok, diff = _check_output_correct(_viz_turn(expected=_viz(sort=sort)), _result(viz=_viz())) @@ -1273,7 +1283,7 @@ def test_output_correct_checks_sort_only_when_expected_states_it(): assert diff[0].startswith("sort") -def test_output_correct_sort_without_a_direction_matches_either_direction(): +def test_output_correct_sort_without_a_direction_matches_either_direction() -> None: by_units = [{"type": "metric_sort", "metrics": ["m"]}] asc = [{"type": "metric_sort", "direction": "ASC", "metrics": ["m"]}] assert _check_output_correct(_viz_turn(expected=_viz(sort=by_units)), _result(viz=_viz(sort=asc)))[0] is True @@ -1281,11 +1291,11 @@ def test_output_correct_sort_without_a_direction_matches_either_direction(): assert _check_output_correct(_viz_turn(expected=_viz(sort=desc)), _result(viz=_viz(sort=asc)))[0] is False -def test_output_correct_is_unknown_when_nothing_is_expected(): +def test_output_correct_is_unknown_when_nothing_is_expected() -> None: assert _check_output_correct(_viz_turn(), _result(viz=_viz())) == (None, []) -def test_output_correct_compares_tool_arguments_across_the_turn(): +def test_output_correct_compares_tool_arguments_across_the_turn() -> None: turn = TurnDefinition( turn_id="a", message="alert me", @@ -1300,7 +1310,7 @@ def test_output_correct_compares_tool_arguments_across_the_turn(): assert _check_output_correct(turn, _result(), [wrong])[0] is False -def test_output_correct_compares_maql_for_a_metric_turn(): +def test_output_correct_compares_maql_for_a_metric_turn() -> None: turn = TurnDefinition( turn_id="m", message="make it", @@ -1331,11 +1341,11 @@ def test_output_correct_compares_maql_for_a_metric_turn(): (_result(text="Pick one", unhandledParts=[{"type": "clarifyingQuestions"}]), "question"), ], ) -def test_classify_reply(result, kind): +def test_classify_reply(result: ChatResult, kind: str) -> None: assert classify_reply(result, result.text_response or render_answer_text(result)) == kind -def test_resolve_conversation_mode(monkeypatch): +def test_resolve_conversation_mode(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("GD_EVAL_CONVERSATION_MODE", raising=False) assert resolve_conversation_mode() == "legacy" monkeypatch.setenv("GD_EVAL_CONVERSATION_MODE", "context") @@ -1350,11 +1360,11 @@ class _FakeJudge: model_name = "fake-judge" - def __init__(self, lost_context): + def __init__(self, lost_context: bool | None) -> None: self.lost_context = lost_context - self.calls = [] + self.calls: list[tuple[str, str, str]] = [] - def judge(self, history, latest, question): + def judge(self, history: str, latest: str, question: str) -> ClarificationVerdict: self.calls.append((history, latest, question)) return ClarificationVerdict(lost_context=self.lost_context, reasoning="because") @@ -1362,7 +1372,9 @@ def judge(self, history, latest, question): _SKILLS = _tool("set_skills", {"skill_names": ["visualization"]}) -def _run(replies, turns, mode="context", judge=None, **kw): +def _run( + replies: list, turns: list[TurnDefinition], mode: str = "context", judge: _FakeJudge | None = None, **kw: object +) -> tuple[ConversationResult, MagicMock]: client = MagicMock() client.create_conversation.return_value = "conv-1" client.send_message.side_effect = replies @@ -1377,7 +1389,7 @@ def _run(replies, turns, mode="context", judge=None, **kw): return result, client -def test_context_mode_fails_a_turn_that_asks_for_what_the_conversation_established(): +def test_context_mode_fails_a_turn_that_asks_for_what_the_conversation_established() -> None: turns = [ _viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz(filters=_EXPRESS), message="its margin?", depends_on=["t1"]), @@ -1414,7 +1426,7 @@ def test_context_mode_fails_a_turn_that_asks_for_what_the_conversation_establish assert question == "Which store format do you mean?" -def test_context_mode_sends_the_set_answer_to_a_legitimate_question(): +def test_context_mode_sends_the_set_answer_to_a_legitimate_question() -> None: turns = [_viz_turn("t1", expected=_viz(), message="revenue by format", set_answers=["Gross Margin Percent."])] result, client = _run( [_result(text="Which metric do you mean?"), _result(viz=_viz(), tools=[_SKILLS])], @@ -1426,7 +1438,7 @@ def test_context_mode_sends_the_set_answer_to_a_legitimate_question(): assert result.turn_results[0].clarifications[0].reply == "Gross Margin Percent." -def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_against_the_turn(): +def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_against_the_turn() -> None: turns = [_viz_turn("t1", expected=_viz())] judge = _FakeJudge(lost_context=True) result, client = _run( @@ -1437,7 +1449,7 @@ def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_agai assert result.turn_results[0].context_success is True -def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges(): +def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges() -> None: turns = [_viz_turn("t1", expected=_viz())] incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations") result, client = _run([incomplete, _result(viz=_viz(), tools=[_SKILLS])], turns, judge=_FakeJudge(False)) @@ -1449,14 +1461,14 @@ def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges(): assert result.stalled_turns == 1 -def test_context_mode_never_primes_the_simulated_user_with_the_expected_output(): +def test_context_mode_never_primes_the_simulated_user_with_the_expected_output() -> None: turns = [_viz_turn("t1", expected=_viz())] with patch("gooddata_eval.core.agentic.conversation._get_sim_user_response") as legacy_sim: _run([_result(text="Which metric?"), _result(viz=_viz(), tools=[_SKILLS])], turns, judge=_FakeJudge(False)) legacy_sim.assert_not_called() -def test_legacy_mode_keeps_the_primed_simulated_user_and_the_legacy_verdict(): +def test_legacy_mode_keeps_the_primed_simulated_user_and_the_legacy_verdict() -> None: turns = [_viz_turn("t1", expected=_viz(filters=_EXPRESS))] with patch("gooddata_eval.core.agentic.conversation._get_sim_user_response", return_value="Express") as sim: result, _ = _run( @@ -1471,7 +1483,7 @@ def test_legacy_mode_keeps_the_primed_simulated_user_and_the_legacy_verdict(): assert result.turn_results[0].output_correct is False -def test_context_kept_rate_is_over_labelled_turns_only(): +def test_context_kept_rate_is_over_labelled_turns_only() -> None: turns = [ _viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz(), depends_on=["t1"]), @@ -1485,7 +1497,7 @@ def test_context_kept_rate_is_over_labelled_turns_only(): assert result.context_success is False -def test_fresh_conversation_per_turn_gives_every_turn_an_empty_history(): +def test_fresh_conversation_per_turn_gives_every_turn_an_empty_history() -> None: turns = [_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz())] good = _result(viz=_viz(), tools=[_SKILLS]) _, client = _run([good, good], turns, judge=_FakeJudge(False), fresh_conversation_per_turn=True) @@ -1493,7 +1505,7 @@ def test_fresh_conversation_per_turn_gives_every_turn_an_empty_history(): assert client.delete_conversation.call_count == 2 -def test_cleanup_deletes_created_metrics_and_alerts_but_keeps_a_metric_updated_in_place(): +def test_cleanup_deletes_created_metrics_and_alerts_but_keeps_a_metric_updated_in_place() -> None: turns = [_viz_turn("t1", expected=_viz())] tools = [ _SKILLS, @@ -1510,7 +1522,7 @@ def test_cleanup_deletes_created_metrics_and_alerts_but_keeps_a_metric_updated_i assert [c.args[2] for c in del_alert.call_args_list] == ["alert-1"] -def test_cleanup_still_runs_for_a_metric_created_before_the_stream_died(): +def test_cleanup_still_runs_for_a_metric_created_before_the_stream_died() -> None: turns = [_viz_turn("t1", expected=_viz())] partial = _result(tools=[_tool("create_metric", result={"data": {"metric_id": "orphan", "maql": "SELECT 1"}})]) with ( @@ -1521,7 +1533,7 @@ def test_cleanup_still_runs_for_a_metric_created_before_the_stream_died(): assert [c.args[2] for c in del_metric.call_args_list] == ["orphan"] -def test_context_mode_pushes_a_stalled_turn_once_then_ends_it(): +def test_context_mode_pushes_a_stalled_turn_once_then_ends_it() -> None: turns = [_viz_turn("t1", expected=_viz())] stall = _result(text="Let me look into that.") result, client = _run([stall, stall, stall], turns, judge=_FakeJudge(False)) @@ -1531,7 +1543,7 @@ def test_context_mode_pushes_a_stalled_turn_once_then_ends_it(): @pytest.mark.parametrize(("mode", "gate"), [("legacy", True), ("context", False)]) -def test_scores_carry_both_verdicts_and_gate_on_the_mode(mode, gate): +def test_scores_carry_both_verdicts_and_gate_on_the_mode(mode: str, gate: bool) -> None: """A turn that shows the chart but drops an established filter: legacy passes it, context does not.""" client = MagicMock() client.create_conversation.return_value = "conv-1" @@ -1539,9 +1551,9 @@ def test_scores_carry_both_verdicts_and_gate_on_the_mode(mode, gate): fixture = ConversationFixture( id="c", expected_skills=["visualization"], turns=[_viz_turn("t1", expected=_viz(filters=_EXPRESS))] ) - captured = {} + captured: dict = {} - def _capture(_submit, _identity, **kwargs): + def _capture(_submit: object, _identity: object, **kwargs: object) -> None: captured["write_scores"] = kwargs["write_scores"] with ( @@ -1574,7 +1586,7 @@ def _capture(_submit, _identity, **kwargs): assert scores["turns_before_first_break"] == 0 -def test_output_correct_treats_an_end_date_after_today_as_today(): +def test_output_correct_treats_an_end_date_after_today_as_today() -> None: """ "2025 and 2026 so far": the year end and today select the same data.""" so_far = { "f": {"type": "date_filter", "using": "dataset/transaction_date", "from": "2025-01-01", "to": "2026-12-31"} @@ -1593,7 +1605,7 @@ def test_output_correct_treats_an_end_date_after_today_as_today(): ] -def _llm_returning(body): +def _llm_returning(body: str) -> _ClarificationLLM: llm = _ClarificationLLM.__new__(_ClarificationLLM) llm.model = "fake" llm._system_prompt = "" @@ -1603,7 +1615,7 @@ def _llm_returning(body): return llm -def test_clarification_llm_reads_the_named_boolean(): +def test_clarification_llm_reads_the_named_boolean() -> None: assert _llm_returning('{"already_in_conversation": true, "reasoning": "t2 said so"}').already_in_conversation( "p" ) == (True, "t2 said so") @@ -1613,16 +1625,16 @@ def test_clarification_llm_reads_the_named_boolean(): @pytest.mark.parametrize("body", ['{"score": 0}', '{"already_in_conversation": "yes"}', "not json", ""]) -def test_clarification_llm_refuses_anything_but_a_boolean_verdict(body): +def test_clarification_llm_refuses_anything_but_a_boolean_verdict(body: str) -> None: with pytest.raises(JudgeResponseError): _llm_returning(body).already_in_conversation("p") -def test_judge_prompt_marks_an_empty_history(): +def test_judge_prompt_marks_an_empty_history() -> None: assert "CONVERSATION BEFORE:\n(empty)" in judge_prompt("", "Sort it", "By what?") -def test_context_mode_restates_at_most_once_per_turn(): +def test_context_mode_restates_at_most_once_per_turn() -> None: turns = [_viz_turn("t1", expected=_viz(), set_answers=["Gross Margin Percent."])] with patch( "gooddata_eval.core.agentic.conversation.reply_restating", return_value="As I said earlier, X." @@ -1636,7 +1648,7 @@ def test_context_mode_restates_at_most_once_per_turn(): assert client.send_message.call_args_list[2].args[1] == "Gross Margin Percent." -def test_output_correct_treats_a_single_value_in_as_equals(): +def test_output_correct_treats_a_single_value_in_as_equals() -> None: turn = TurnDefinition( turn_id="m", message="make it", @@ -1660,7 +1672,7 @@ def test_output_correct_treats_a_single_value_in_as_equals(): assert _check_output_correct(turn, _result(), [two])[0] is False -def test_an_iteration_limit_after_the_chart_was_made_keeps_the_chart(): +def test_an_iteration_limit_after_the_chart_was_made_keeps_the_chart() -> None: turns = [_viz_turn("t1", expected=_viz())] partial = _result(viz=_viz(), tools=[_SKILLS]) incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations", partial_result=partial) @@ -1671,7 +1683,7 @@ def test_an_iteration_limit_after_the_chart_was_made_keeps_the_chart(): assert turn.stalled is True -def test_a_fresh_conversation_starts_with_no_active_skills(): +def test_a_fresh_conversation_starts_with_no_active_skills() -> None: turns = [_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz())] result, _ = _run( [_result(viz=_viz(), tools=[_SKILLS]), _result(viz=_viz())], @@ -1682,7 +1694,7 @@ def test_a_fresh_conversation_starts_with_no_active_skills(): assert result.turn_results[1].skill_routing is False -def test_a_question_on_the_last_allowed_round_is_still_judged(): +def test_a_question_on_the_last_allowed_round_is_still_judged() -> None: turns = [_viz_turn("t1", expected=_viz())] judge = _FakeJudge(lost_context=True) result, _ = _run([_result(text="Which chart?")], turns, judge=judge, max_clarification_turns=0) @@ -1690,14 +1702,14 @@ def test_a_question_on_the_last_allowed_round_is_still_judged(): assert result.lost_context_clarifications == 1 -def test_not_in_is_not_rewritten_as_equals(): +def test_not_in_is_not_rewritten_as_equals() -> None: assert "not in" in _canonical_maql('SELECT {metric/a} WHERE {label/b} NOT IN ("x")') assert _canonical_maql('SELECT {metric/a} WHERE {label/b} IN ("x")') == _canonical_maql( 'SELECT {metric/a} WHERE {label/b} = "x"' ) -def test_the_flat_expected_shape_is_still_checked(): +def test_the_flat_expected_shape_is_still_checked() -> None: turn = TurnDefinition( turn_id="t", message="m", @@ -1711,13 +1723,13 @@ def test_the_flat_expected_shape_is_still_checked(): # --- clarification judge ------------------------------------------------------------- -def _judge_with(llm): +def _judge_with(llm: MagicMock) -> ClarificationJudge: judge = ClarificationJudge(model="fake") judge._llm = llm return judge -def test_judge_reads_the_verdict_and_caches_it_per_prompt(): +def test_judge_reads_the_verdict_and_caches_it_per_prompt() -> None: llm = MagicMock() llm.already_in_conversation.return_value = (True, "t1 named it") judge = _judge_with(llm) @@ -1729,7 +1741,7 @@ def test_judge_reads_the_verdict_and_caches_it_per_prompt(): assert llm.already_in_conversation.call_count == 2 -def test_an_unreadable_verdict_leaves_the_clarification_unjudged(): +def test_an_unreadable_verdict_leaves_the_clarification_unjudged() -> None: llm = MagicMock() llm.already_in_conversation.side_effect = JudgeResponseError("no boolean") verdict = _judge_with(llm).judge("", "x", "y") @@ -1737,7 +1749,9 @@ def test_an_unreadable_verdict_leaves_the_clarification_unjudged(): assert "no boolean" in verdict.error -def test_a_provider_fault_leaves_clarifications_unjudged_and_is_announced_once(capsys): +def test_a_provider_fault_leaves_clarifications_unjudged_and_is_announced_once( + capsys: pytest.CaptureFixture[str], +) -> None: llm = MagicMock() llm.already_in_conversation.side_effect = RuntimeError("401 bad key") judge = _judge_with(llm) @@ -1746,23 +1760,24 @@ def test_a_provider_fault_leaves_clarifications_unjudged_and_is_announced_once(c assert capsys.readouterr().out.count("left unjudged") == 1 -def test_the_judge_needs_no_api_key_until_it_is_asked(monkeypatch): +def test_the_judge_needs_no_api_key_until_it_is_asked(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.delenv("OPENAI_API_KEY", raising=False) judge = ClarificationJudge() assert judge.model_name assert judge.judge("", "a", "q").error is not None -def test_the_clarification_llm_uses_its_own_prompt(monkeypatch): +def test_the_clarification_llm_uses_its_own_prompt(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + _fake_openai(monkeypatch, MagicMock()) llm = _ClarificationLLM(model="gpt-4o") assert "already_in_conversation" in llm._system_prompt assert llm.model == "gpt-4o" -def test_the_clarification_llm_retries_an_empty_body_once(): +def test_the_clarification_llm_retries_an_empty_body_once() -> None: llm = _llm_returning("") - calls = [] + calls: list[int] = [] choice = MagicMock() choice.message.content = '{"already_in_conversation": false, "reasoning": "r"}' bodies = iter([MagicMock(choices=[]), MagicMock(choices=[choice])]) @@ -1774,7 +1789,13 @@ def test_the_clarification_llm_retries_an_empty_body_once(): # --- simulated user (context mode) ------------------------------------------------------- -def _openai_returning(content): +def _fake_openai(monkeypatch: pytest.MonkeyPatch, openai_cls: MagicMock) -> MagicMock: + """Stand in for the optional openai package, whether or not it is installed.""" + monkeypatch.setitem(sys.modules, "openai", types.SimpleNamespace(OpenAI=openai_cls)) + return openai_cls + + +def _openai_returning(content: str | None) -> MagicMock: response = MagicMock() response.choices = [MagicMock()] response.choices[0].message.content = content @@ -1783,24 +1804,24 @@ def _openai_returning(content): return MagicMock(return_value=client) -def test_reply_from_facts_without_facts_needs_no_llm(): - with patch("openai.OpenAI") as openai_cls: - assert reply_from_facts("USER: x", "Which?", []) == NO_ANSWER_REPLY +def test_reply_from_facts_without_facts_needs_no_llm(monkeypatch: pytest.MonkeyPatch) -> None: + openai_cls = _fake_openai(monkeypatch, MagicMock()) + assert reply_from_facts("USER: x", "Which?", []) == NO_ANSWER_REPLY openai_cls.assert_not_called() -def test_reply_from_facts_answers_from_the_facts(monkeypatch): +def test_reply_from_facts_answers_from_the_facts(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("OPENAI_API_KEY", "sk-test") - with patch("openai.OpenAI", _openai_returning(" Sales Order Revenue. ")) as openai_cls: - assert reply_from_facts("USER: revenue by month", "Which revenue?", ["Sales Order Revenue."]) == ( - "Sales Order Revenue." - ) + openai_cls = _fake_openai(monkeypatch, _openai_returning(" Sales Order Revenue. ")) + assert reply_from_facts("USER: revenue by month", "Which revenue?", ["Sales Order Revenue."]) == ( + "Sales Order Revenue." + ) prompt = openai_cls.return_value.chat.completions.create.call_args.kwargs["messages"][1]["content"] assert "Sales Order Revenue." in prompt and "Which revenue?" in prompt @pytest.mark.parametrize("failure", ["no_key", "provider_error", "empty_body"]) -def test_simulated_user_falls_back_when_the_llm_gives_nothing(monkeypatch, failure): +def test_simulated_user_falls_back_when_the_llm_gives_nothing(monkeypatch: pytest.MonkeyPatch, failure: str) -> None: if failure == "no_key": monkeypatch.delenv("OPENAI_API_KEY", raising=False) openai_cls = _openai_returning("unused") @@ -1809,39 +1830,39 @@ def test_simulated_user_falls_back_when_the_llm_gives_nothing(monkeypatch, failu openai_cls = _openai_returning(None) if failure == "provider_error": openai_cls.return_value.chat.completions.create.side_effect = RuntimeError("503") - with patch("openai.OpenAI", openai_cls): - assert reply_from_facts("h", "q", ["fact"]) == NO_ANSWER_REPLY - assert reply_restating("h", "q").startswith("As I said earlier") + _fake_openai(monkeypatch, openai_cls) + assert reply_from_facts("h", "q", ["fact"]) == NO_ANSWER_REPLY + assert reply_restating("h", "q").startswith("As I said earlier") -def test_reply_restating_returns_the_llm_restatement(monkeypatch): +def test_reply_restating_returns_the_llm_restatement(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("OPENAI_API_KEY", "sk-test") - with patch("openai.OpenAI", _openai_returning("As I said earlier, Express.")): - assert reply_restating("USER: weakest? ASSISTANT: Express", "Which format?") == "As I said earlier, Express." + _fake_openai(monkeypatch, _openai_returning("As I said earlier, Express.")) + assert reply_restating("USER: weakest? ASSISTANT: Express", "Which format?") == "As I said earlier, Express." -def test_simulated_user_reports_a_missing_openai_package(monkeypatch): - monkeypatch.setitem(__import__("sys").modules, "openai", None) +def test_simulated_user_reports_a_missing_openai_package(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setitem(sys.modules, "openai", None) assert ctx_mod._chat("s", "u") is None # --- turn verdict and content-check edges ------------------------------------------------- -def test_failure_reasons_name_an_unrun_turn_and_an_inactive_skill(): +def test_failure_reasons_name_an_unrun_turn_and_an_inactive_skill() -> None: unrun = _turn_result().model_copy(update={"no_error": False}) assert unrun.failure_reasons()[0] == "turn could not run" wrong_skill = _turn_result().model_copy(update={"skill_routing": False}) assert wrong_skill.failure_reasons() == ["skill 'visualization' not active"] -def test_expected_viz_is_none_for_a_shape_it_cannot_read(): +def test_expected_viz_is_none_for_a_shape_it_cannot_read() -> None: assert _expected_viz({"visualization": "not a chart"}) is None assert _expected_viz({"visualization": {"query": {"fields": {"m": 1}}}}) is None assert _expected_viz({"visualization": {"query": {}}}) is None -def test_mismatches_name_the_dimension_and_the_type(): +def test_mismatches_name_the_dimension_and_the_type() -> None: expected = {**_viz(), "type": "line_chart"} ok, diff = _check_output_correct(_viz_turn(expected=expected), _result(viz=_viz(dim="label/dim_brand.brand_name"))) assert ok is False @@ -1849,7 +1870,7 @@ def test_mismatches_name_the_dimension_and_the_type(): assert any(d.startswith("type") for d in diff) -def test_output_correct_without_anything_to_compare_against(): +def test_output_correct_without_anything_to_compare_against() -> None: assert _check_output_correct(_viz_turn(expected=_viz()), _result()) == (False, ["no visualization to compare"]) metric_turn = TurnDefinition( turn_id="m", @@ -1863,7 +1884,7 @@ def test_output_correct_without_anything_to_compare_against(): assert _check_output_correct(no_maql, _result(), []) == (None, []) -def test_cleanup_ignores_tool_results_it_cannot_read(): +def test_cleanup_ignores_tool_results_it_cannot_read() -> None: unreadable = [ ToolCallEvent.model_validate(_tool("create_metric", result=["not", "a", "dict"])), ToolCallEvent.model_validate(_tool("create_metric", result={"data": {"isError": True}})), @@ -1874,7 +1895,7 @@ def test_cleanup_ignores_tool_results_it_cannot_read(): assert _created_alert_ids(unreadable) == ["a-nested"] -def test_fresh_conversation_per_turn_refuses_a_conversation_it_did_not_create(): +def test_fresh_conversation_per_turn_refuses_a_conversation_it_did_not_create() -> None: fixture = ConversationFixture(id="c", expected_skills=["visualization"], turns=[_viz_turn("t1")]) with ( patch("gooddata_eval.core.agentic.conversation.ChatClient"), @@ -1891,7 +1912,7 @@ def test_fresh_conversation_per_turn_refuses_a_conversation_it_did_not_create(): ) -def test_context_kept_rate_is_scored_when_there_are_dependent_turns(): +def test_context_kept_rate_is_scored_when_there_are_dependent_turns() -> None: client = MagicMock() client.create_conversation.return_value = "conv-1" client.send_message.side_effect = [_result(viz=_viz(), tools=[_SKILLS]), _result(viz=_viz())] @@ -1900,7 +1921,7 @@ def test_context_kept_rate_is_scored_when_there_are_dependent_turns(): expected_skills=["visualization"], turns=[_viz_turn("t1", expected=_viz()), _viz_turn("t2", expected=_viz(), depends_on=["t1"])], ) - captured = {} + captured: dict = {} with ( patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=client), patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 418c6e04e..59b2fdff6 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -857,10 +857,10 @@ def _capture(_submit, _identity, **kwargs): assert isinstance(scores["steps"], int) -def test_extract_created_metric_ids_skips_a_metric_updated_in_place(): +def test_extract_created_metric_ids_skips_a_metric_updated_in_place() -> None: """create_metric is an upsert; deleting an updated metric would delete one that existed before.""" - def call(mid, created_new): + def call(mid: str, created_new: bool) -> ToolCallEvent: return ToolCallEvent.model_validate( { "functionName": "create_metric", diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index 250a8393f..c109ac391 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -498,7 +498,7 @@ def test_parse_sse_lines_metadata_sync_marker_in_malformed_json_is_transient(): @pytest.mark.parametrize("reason", ["max_iterations", "max_tokens", "content_filter"]) -def test_parse_sse_lines_turn_incomplete_is_not_transient(reason): +def test_parse_sse_lines_turn_incomplete_is_not_transient(reason: str) -> None: """gen-ai's "no final answer" error is a 502 with a reason; retrying it would resend the question.""" lines = [ 'data: {"item": {"role": "assistant", "content": {"type": "text", "text": "Let me check."}}}', @@ -513,7 +513,7 @@ def test_parse_sse_lines_turn_incomplete_is_not_transient(reason): @pytest.mark.parametrize("reason", [None, "unknown"]) -def test_parse_sse_lines_502_without_a_definite_reason_stays_transient(reason): +def test_parse_sse_lines_502_without_a_definite_reason_stays_transient(reason: str | None) -> None: payload = {"statusCode": 502, "detail": "LLM provider is not responding"} if reason: payload["reason"] = reason